Add codestyle to handle unnecessary parentheses. #21316

CyrusNajmabadi · 2017-08-04T01:30:33Z

Fixes #25554

CyrusNajmabadi · 2017-08-04T02:00:24Z

Ready for initial review. Need to do VB side.

CyrusNajmabadi · 2017-08-04T02:00:32Z

Tagging @dotnet/roslyn-ide

CyrusNajmabadi · 2017-08-04T10:17:17Z

Tagging @DustinCampbell

CyrusNajmabadi · 2017-08-04T19:22:10Z

@DustinCampbell User option looks like this:

'Always' means we don't touch any parens and we don't give you any notification about them.

"When clarifying precedence" means we'll ignore extraneous parens in arithmetic statements (i.e. we won't notify you about (a * b) + (c * d). While unnecessary, including these types of parens in arithmetic expressions is valuable for clarity. We will fade out for things like the parens in (this).ToString() though. Here the parens add nothing and really can/should be removed. By default we only fade these out (no messages/suggestions/squiggles). Users can opt into stronger notifications for these, or htey can switch to 'Always' to say "i want to parenthesize however i like, don't tell me about it".

Tagging @kuhlenh as well.

CyrusNajmabadi · 2017-08-05T19:36:46Z

tagging @Pilchie

sharwell · 2017-08-06T19:28:35Z

I'm pretty strongly opposed to the inclusion of Never as an option here. I believe it has known problems in real-world scenarios, and will only serve to reduce program quality. If someone really wants to have such a feature in their code, they could easily do so as part of a third-party analyzer. I do not believe we should include the feature out-of-the-box, as an analyzer or refactoring, now or ever.

CyrusNajmabadi · 2017-08-07T05:43:45Z

I disagree. As with any of our options, users can choose to opt into styles of coding that others may feel reduce quality and may potentially ease the introduction of bugs into code. This is, for example, why we have two different coding styles in Roslyn itself as there are styles we offer that this team believes have a negative effect on quality.

Similarly, the usage of "out vars" as we use them (and the code style feature we offer around them) is one that several community members mentioned they absolutely would not use and felt would lower code quality. That's why these are options and styles. They're not for everyone. They're made available so that people who happen to prefer such an approach have the facility to have the IDE handle it for them. I see no reason to make people go to a third party analyzer here when this is a reasonable thing for them to want for their own code.

Tagging @kuhlenh

CyrusNajmabadi · 2017-08-10T02:49:39Z

@dpoeschl This now has our formalization and i would like your eyes here.

Not implemented (but hopefully will be tonight): Adding in required parentheses based on hte users' options.

CyrusNajmabadi · 2017-08-10T04:40:42Z

@dpoeschl have added fixer to add parentheses as necessary

CyrusNajmabadi · 2017-08-10T05:33:06Z

Feature appears to the user (with these defaults) as:

The idea is, by default, we don't require, or try to remove, parentheses in logical or arithmetic operations. However, by default we will fade out unnecessary parens for other situations. i.e. if the user writes var = (a).length, we will fade out those parens and offer to remove them. These parens are purely unnecessary and do not aid precedence clarity.

The options for 'arithmetic' and 'logical' are both:

'Ignore' means 'do not do anything with parentheses used in the expression.
'Require for precedence clarity' means add parentheses for cases like 1 + 1 << 2 to change it into (1 + 1) << 2. The latter is just how the compiler parsed the former, but now the parentheses make it clearer that that was hte parsing vs the incorrect interpretation of 1 + (1 << 2).
'Remove if unnecessary' means that the compiler will remove parens if they don't affect the runtime result of the operation. i.e. if you have 1 + (2 * 3) it will suggest converting that into 1 + 2 * 3

We've separated out arithmetic and logical operations as there has been feedback (i.e. from @dpoeschl`) that he's fine with arithmetic operations as is, but would personally like to be able to opt into stricter rules around how || and && operations are parenthesized.

The options for 'other' are:

CyrusNajmabadi · 2017-08-10T05:41:36Z

@dotnet/roslyn-ide This is ready for review.

dpoeschl · 2017-08-11T17:57:34Z

...table/RemoveUnnecessaryParentheses/AbstractRemoveUnnecessaryParenthesesDiagnosticAnalyzer.cs

+
+            var additionalLocations = ImmutableArray.Create(parenthesizedExpression.GetLocation());
+            context.ReportDiagnostic(
+                Diagnostic.Create(this.UnnecessaryWithSuggestionDescriptor, parenthesizedExpression.GetFirstToken().GetLocation(), additionalLocations));


I understand the desire for the parentheses to be grayed out, but this doesn't honor the user's chosen severity. If I want these to be warnings, then I set the severity to "Warning" and that choice is ignored. With things like Conditional Delegate Call, we gray out the unused bit but also honor the chosen Severity by squiggling the invocation.

this has been fixed.

dpoeschl · 2017-08-11T18:07:26Z

...res/Core/Portable/AddRequiredParentheses/AbstractAddRequiredParenthesesDiagnosticAnalyzer.cs

+                ? arithmeticPreference.Notification.Value
+                : logicalPreference.Notification.Value;
+
+            var operatorToken = binaryExpression.ChildNodesAndTokens()[1];


I think this will end up being a bit weird. If you have a || b && c && d, this will squiggle only the final && even though it plans to parenthesize all of (b && c && d). I mean, it makes sense when you think about it, but to someone who doesn't automatically visualize syntax trees that correctly honor left associativity, it might be misleading. Should we have one diagnostic bug squiggle all of the operators that will get grouped together? How's the editor support of that?

this has been changed.

dpoeschl · 2017-08-11T18:09:12Z

src/EditorFeatures/CSharpTest/AddRequiredParentheses/AddRequiredParenthesesTests.cs

+{
+    void M()
+    {
+        int x = a || b [||]&& c;


Wow, the test caret position [||] is really hard to read in this scenario. Can we switch to $$ (since it's a diagnostic and not a refactoring)?

unfortunately, that would take some deep manipulation of hte unit test harness. i'm ok keeping as is :)

Actually, turns out it was easy. So i've added support for that and changed all these tests to use $$ now.

dpoeschl · 2017-08-11T18:12:26Z

src/EditorFeatures/CSharpTest/AddRequiredParentheses/AddRequiredParenthesesTests.cs

+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddRequiredParentheses)]
+        public async Task TestNotIfArithmeticPrecedenceIsOff()


I had a hard time parsing the *IsOff() unit test names. Perhaps *IsNotEnforced?

dpoeschl · 2017-08-11T18:13:58Z

src/EditorFeatures/CSharpTest/AddRequiredParentheses/AddRequiredParenthesesTests.cs

+{
+    void M()
+    {
+        int x = 1 + 2 [||]+ 3;


Wait, even if Arithmetic Precedence enforcement is on, this still wouldn't report anything so this test isn't actually testing anything, right? (Identical code to the previous test that was already showing it not reporting with everything enabled... so of course it wouldn't report with a subset of things enabled.)

I'm keeping this test (just for exhaustiveness), but i'm adding another test that actually verifies that we don't add teh parens where we normally would if hte optoin was on.

dpoeschl · 2017-08-11T20:16:37Z

...table/RemoveUnnecessaryParentheses/AbstractRemoveUnnecessaryParenthesesDiagnosticAnalyzer.cs

+
+            if (!ShouldRemoveParentheses())
+            {
+                // Parentheses should not be removed.


Unnecessary comment?

dpoeschl · 2017-08-11T20:19:51Z

src/VisualStudio/Core/Impl/Options/Style/EnumCodeStyleOptionViewModel.cs

@@ -42,6 +58,21 @@ static EnumCodeStyleOptionViewModel()
            OptionSet options,
            string groupName,
            List<CodeStylePreference> preferences)
+            : this(option, null, description, enumValues, previews, info,


language: null (I know it's annoying to have to do all the other ones too... For now...)

dpoeschl · 2017-08-11T20:25:19Z

src/EditorFeatures/CSharpTest/AddRequiredParentheses/AddRequiredParenthesesTests.cs

+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddRequiredParentheses)]
+        public async Task TestNotIfLogicalPrecedenceIsOff()


Same as the other "IsOff", I don't think it's testing anything.

dpoeschl · 2017-08-11T20:29:47Z

src/EditorFeatures/CSharpTest/RemoveUnnecessaryParentheses/RemoveUnnecessaryParenthesesTests.cs

+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryParentheses)]
+        public async Task TestArithmeticNotRequiredForClarityWhenPrecedenceStaysTheSame1()


As discussed elsewhere, the precedence doesn't change but the order of evaluation does, which can be dangerous in several cases.

dpoeschl · 2017-08-11T20:49:21Z

src/Workspaces/Core/Portable/Shared/Extensions/ParenthesizedExpressionSyntaxExtensions.cs

+            // -- even if they could be removed depending on whether the parenthesized
+            // expression appears on the left or right side of the parent binary expression.
+
+            // First, does the binary expression result in an operator overload being 


Test for this?

tests were added for this.

dpoeschl · 2017-08-11T20:58:03Z

Super contrived, but FixAll breaks this:

[EDIT: This case is interesting because neither set of parentheses is necessary independently, but one set needs to be kept by FixAll]

dpoeschl · 2017-08-11T21:15:12Z

This is a case that probably needs to be fixed before it could be adopted: It wants to add parentheses to goo != null && bar != null for clarity...
Or goo() && bar != null
Or goo() && bar is SomeType
Or if (reference == null || _offset + size > _fileSize)

Discussed offline: maybe logical means "both the parent and child are logical; same with arithmetic (a > b || c < d)"

sharwell · 2017-08-11T21:28:52Z

➡️ Let's remove the two options "Remove if unnecessary" from both arithmetic and logical. We can discuss their addition in a later release, but at this time I remain so strongly opposed to their existence (even though they are not the default) that it would appear to undermine the feature as a whole.

sharwell · 2017-08-11T21:29:39Z

Super contrived, but FixAll breaks this:

StyleCop Analyzers had a user hit this case in the wild (DotNetAnalyzers/StyleCopAnalyzers#1284). Part of the reason I keep suggesting new features start with the existing test cases we have, because they cover many things like this.

Edit: I see, the interesting thing here is that one set of parentheses can be removed, but not both. We had a similar case arise with converting default(T) to default but now I can't find it.

CyrusNajmabadi · 2017-08-11T21:34:17Z

Let's remove the two options "Remove if unnecessary" from both arithmetic and logical. We can discuss their addition in a later release, but at this time I remain so strongly opposed to their existence (even though they are not the default) that it would appear to undermine the feature as a whole.

I would prefer to keep them. I think it's fine to allow users to opt into this level of cleanup for their own code.

Pilchie · 2017-08-11T22:21:23Z

There's no rush here. Let's wait the weekend and have a discussion about it on Monday.

CyrusNajmabadi · 2017-08-11T23:17:27Z

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxEditorExtensions.cs

+{
+    internal static class SyntaxEditorExtension
+    {
+        public static async Task ApplySemanticEditsAsync<TNode>(


Tagging @sharwell

sharwell · 2018-04-05T18:40:56Z

I'd also be very interested in your thougths here.

The pull request adds more than 1,000 different configurations. It's overwhelming, and worse, looking at the options I have no idea what the selections will actually mean for code.

Can you provide examples of why the following simplified configuration would not be sufficient?

Expressions with operators in the same category
- Options: ignore, remove if unnecessary, add to clarify precedence (default)
Expressions with operators in different categories
- Options: ignore, add to clarify precedence (default)
Other unnecessary parentheses
- Options: ignore, remove (default)
Exclusions from consideration (i.e. not exposed as user options)
- The top level assignment expression of an expression statement (always treated as same category as right-hand child expression)
- Ternary operators (always 'ignore')
- Null coalescing operator (always 'ignore')
- Unary operators (always treated as same category as parent expression)

CyrusNajmabadi · 2018-04-05T19:00:23Z

Can you provide examples of why the following simplified configuration would not be sufficient?

Primarily, because i think it would be too coarse-grained. For example, let's look at this approach:

Expressions with operators in the same category
Options: ignore, add to clarify precedence (default)

this means if i'm a person who is totally fine with differing arithmetic precedences (i.e. "i'm fine with a + b * c"), but i don't like mixing logical precedence (i.e. "i'm not fine with a || b && c"), there's no way for me to express that.

It seems like a very big pill to have to swallow if you had to say "i must make all precedence explicit, when really there are only a few cases that i find myself caring about in my project".

The pull request adds more than 1,000 different configurations.

:) It's worth noting that we add, effectively, the same number of configs that resharper adds. You specify which precedences you care about, and you specify if you want the parens for clarity, or you don't want them. They happen to have a nicer UI for this (through the use of a good UI widget), but they allow you to be clear about where you want the tool involved, and where you'd like it left out.

--

Additional notes:

Options: ignore, remove if unnecessary, add to clarify precedence (default)

@kuhlenh Felt strongly that "ignore" didn't fit in our design around code-style. Instead, we've always gone the route of having a default preference in one direction, but having the severity be "none" if we didn't want to subject users to lots of messages in the IDE by default.
I would be very wary about having "clarify precedence" be the default. I imagine that would lead practically every upgrade to end up with an overwhelming number of messages with practically any project (especially if we changed to the model where these categories were very coarse).

Other unnecessary parentheses
Options: ignore, remove (default)

This i feel safe about (and is how the current PR is set up). The 'other' category is really for cases where there is no precedence concept involved at all, and which you can generally feel very safe about removing while not impacting clarity.

Exclusions from consideration (i.e. not exposed as user options)
Ternary operators (always 'ignore')

That's close to how things are today, except if you have something like (a) ? (1) : (2). i.e. if you have a primary expression in any of hte parts, we mark those as unnecessary. But the moment you have anything even slightly more complicated than a primary expression, we don't touch things to allow you to use parentheses for clarity.

CyrusNajmabadi · 2018-04-05T19:05:19Z

looking at the options I have no idea what the selections will actually mean for code.

I think it's worth noting that we have previews for these options, which help explain their meaning. They were also named to help explain how they woudl work. i.e. "dotnet_style_parentheses_in_logical_expressions:always_for_clarity/never_if_unnecessary". We tried to make it so that this read in a way that would make sense to someone. i.e. "in a logical expression, always require parentheses for clarity" or "in a logical expression, never allow parentheses that are unnecessary".

ergo, if i have some logical operator "a op b" then this controls if i would want (or not want) parentheses inside the op to clarify precedence.

The preview demosntrates this by showing the following for 'always_for_clarity':

+        // {ServicesVSResources.Prefer_colon}
+        var v = a || (b && c);
+
+        // {ServicesVSResources.Over_colon}
+        var v = a || b && c;

And the following for 'never_if_unnecessary':

+        // {ServicesVSResources.Prefer_colon}
+        var v = a || b && c;
+
+        // {ServicesVSResources.Over_colon}
+        var v = a || (b && c);

CyrusNajmabadi · 2018-04-05T19:06:38Z

Null coalescing operator (always 'ignore')

Could you clarify this bit? Why would null-coalescing be special cased?

CyrusNajmabadi · 2018-04-05T19:09:48Z

Also, just to be clear: i've very interested in your idea, and i welcome approaches that can potentially simplify things here. If we can reformulate this in a better way, i'm all ears. The above points were just my initial concerns about your proposal. But it's super interesting and maybe with some tweaks could work.

FWIW, i started with an initial approach that was similar to what you had. However, i almost immediately disliked it because of my own personal preferences around where i find precedence parens useful, vs where i personally do not prefer them. @dpoeschl and i went back and forth on a bunch of ideas, but we eventually settled on wanting to try out the 'fine-grained' approach, because it truly was a consistent (if potentially overwhelming) of providing this functionality.

sharwell · 2018-04-05T19:09:53Z

this means if i'm a person who is totally fine with differing arithmetic precedences (i.e. "i'm fine with a + b * c"), but i don't like mixing logical precedence (i.e. "i'm not fine with a || b && c"), there's no way for me to express that.

This sounds like a valuable trade-off. It eliminates an edge case that few users will care strongly about for a dramatic reduction in complexity.

@kuhlenh Felt strongly that "ignore" didn't fit in our design around code-style. Instead, we've always gone the route of having a default preference in one direction, but having the severity be "none" if we didn't want to subject users to lots of messages in the IDE by default.

Sure, this is functionally equivalent to what I was saying so no objection there.

I would be very wary about having "clarify precedence" be the default. I imagine that would lead practically every upgrade to end up with an overwhelming number of messages with practically any project (especially if we changed to the model where these categories were very coarse).

We can do this at severity 'suggestion'. Same as we did for using collection and object initializers.

That's close to how things are today, except if you have something like (a) ? (1) : (2). i.e. if you have a primary expression in any of hte parts, we mark those as unnecessary. But the moment you have anything even slightly more complicated than a primary expression, we don't touch things to allow you to use parentheses for clarity.

That works for me. 👍

CyrusNajmabadi · 2018-04-05T19:11:58Z

This sounds like a valuable trade-off. It eliminates an edge case that few users will care strongly about for a dramatic reduction in complexity.

It would certainly be something we can try out! I would like peopel to then use this and to see if this felt ok. My gut feeling is that it will be too coarse grained... but i woudl be happy to be proven wrong! :)

Neme12 · 2018-04-05T19:12:56Z

I would be very much against having "clarify precedence" be the default with any severity greater than none. If there is no "ignore" option then it has to be one or the other but it should definitely be set to none. People have a lot of differing opinions about this and we can't assume they want 1000s of suggestions in their code every time they used an operator after upgrading.

sharwell · 2018-04-05T19:13:17Z

However, i almost immediately disliked it because of my own personal preferences around where i find precedence parens useful, vs where i personally do not prefer them.

This is interesting to me for three reasons:

I believe your views of operator associativity are influenced by your wildly-above-average experience and position on the language design team, and thus do not relate directly to the average developer or team
I believe you are well-equipped to create your own analyzer to enforce a deviating style
I believe that situations in the wild where (1) does not imply (2) are even more rare than either in isolation

CyrusNajmabadi · 2018-04-05T19:15:04Z

A question on something i missed earlier:

Expressions with operators in different categories
Options: ignore, add to clarify precedence (default)

Was it intentional that there as no "remove if unnecessary here"?

i'm not a fan of htat because i think there are some people who very much would want to remove unnecessary parens from their codebase. It goes into the midset of people who actually find superfluous stuff distracting and unhelpful. i.e. "the zen of minimal code".
i get that this would likely be necessary unless we had the special exceptions for things like "1 + 2 << 3" since htat is so error-prone. It's worth noting that my approach allows people to opt into removing unnecessary parens in places where they're commonly felt to be ok to remove, while steering people away from removing them from cases like shift (since we literally say in the UI: "Remove if unnecessary (not recommended)"). I would also be ok with my approach, while not actually having a UI option, forcing you to just put the stuff in the .editorconfig for "shifts" if you absolutely wanted that.

CyrusNajmabadi · 2018-04-05T19:17:40Z

This is interesting to me for three reasons:

I believe your views of operator associativity are influenced by your wildly-above-average experience and position on the language design team, and thus do not relate directly to the average developer or team

perhaps. it might be interesting ot analyze our own codebase, and see (for example) the percentages on things like "parentheses in arithmetic" versus "parentheses in logical". i.e. in arithmetic expression with differing precedence, what percentage of the time are those parenthesed unnecessarily. And then do the same check for logical.

My gut feeling is that we'll find that logical ops are parenthesized far more often. even in our codebase where we're skilled in this domain and we really know the difference. But even for our own team, we find parens for one more valuable and more common than parens for the other.

sharwell · 2018-04-05T19:18:42Z

Was it intentional that there as no "remove if unnecessary here"?

Yes, this was intentionally omitted.

it might be interesting ot analyze our own codebase

We do have a downside that mixed-precedence is generally discouraged in this codebase for ongoing development.

CyrusNajmabadi · 2018-04-05T19:19:52Z

We do have a downside that mixed-precedence is generally discouraged in this codebase for ongoing development.

Could you clarify how that is a downside?

I think it's extremely interesting that even in roslyn (which likely has the largest set of experts in the languages), we've chosen to still want some of our expressions to err on the side of clarity for certain operations.

…Parentheses

CyrusNajmabadi · 2018-04-05T20:17:49Z

@sharwell @Neme12 and me discussed this at length in gitter: https://gitter.im/dotnet/roslyn?at=5ac675a42b9dfdbc3a5d3477

the takeaway is that there are three important design decisions that need to be made. The design decisions are:

Should we provide fine-grained options to control how parentheses are handled for each of these categories or should we go with something coarser? We came up with two ways to make things coarser. One was to simply collapse all operators as if they were in a single category, and then only have options around if you always wanted parens for precedence, or you never wanted parens for precedence. The other option was to collapse but into the groups "Arithmetic" and "Non-arithmetic". So the choices are "Fine-grained (7 options for c#)", "Coarse (2 options for C#)" and "Coarsest (1 option for C#). It's also worth nothing that "coarsest" this is close to how StyleCopAnalyzers exposes things and that seems to have worked out well for their customer base.
Should we even offer "never_if_unnecessary" as an option when an expression crosses categories. i.e. should a person be allowed to say "i do not want parens in (a - b) == (c * d) or not?
What should the defaults be for our options.

@sharwell will present this at the next IDE design meeting.

CyrusNajmabadi · 2018-04-05T20:23:17Z

my preferences are:

"Fine-grained" or "coarse are likely fine. "Coarsest" is too extreme for me and i believe enough customers will want to cary the behavior around arithmetic/non-arithmetic to not want to collapse them. @Neme12 and @JohanLarsson also seemed to feel that "coarsest" was too strong. It's also worth noting that roslyn itself seems to informally follow that rule a lot.
I think we should allow the option to never have unnecessary parens. If we go the "coarse/coarsest" route, and a person says they never want unnecessary parens, we will probable need to special case "shift" operators as things to not remove as the precedence is extremely surprising and unintuitive for people. That is not a problem with the "fine-grained" approach as we explicitly mark that option as "not recommended" to change. See Add codestyle to handle unnecessary parentheses. #21316 (comment) for picture of this.
I think the defaults should be: for the normal categories: "always_for_clarity:none", and for the 'other' category "never_if_unnecessary:none".

kuhlenh · 2018-04-05T20:25:35Z

@CyrusNajmabadi @sharwell - I am in favor of "fine-grained (7)" because we don't have a good story for versioning .editorconfig options if we wanted to change this later. That said, I can be convinced to do "coarse (2)" if we can show there is no difference via something like this.

And agree with Cyrus's last 2 points in the post above this.

CyrusNajmabadi · 2018-04-05T20:27:24Z

Also worth having for the conversation. Here is how we present things to the user in tools|options wrt to preview:

And how we discourage people from certain options that are likely going to lead to very unintuitive results:

If you pick either coarse/coarsest, i would like some thought put into how we present the information to the user in a clear manner. Thanks!

…Parentheses

CyrusNajmabadi · 2018-05-03T19:22:30Z

Closing out in favor of #26013

CyrusNajmabadi added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 4, 2017

dnfclas added the cla-already-signed label Aug 4, 2017

CyrusNajmabadi removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 4, 2017

CyrusNajmabadi force-pushed the removeUnnecessaryParentheses branch 3 times, most recently from 2bc1137 to 4fd7d7c Compare August 4, 2017 10:16

CyrusNajmabadi force-pushed the removeUnnecessaryParentheses branch 2 times, most recently from f821b3d to c00b31b Compare August 4, 2017 19:15

CyrusNajmabadi force-pushed the removeUnnecessaryParentheses branch from 1eccca5 to 65b8786 Compare August 5, 2017 19:37

dpoeschl previously requested changes Aug 11, 2017

View reviewed changes

CyrusNajmabadi force-pushed the removeUnnecessaryParentheses branch 2 times, most recently from 0bd55e3 to 752be05 Compare August 11, 2017 23:01

CyrusNajmabadi commented Aug 11, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into removeUnnecessary…

09ab977

…Parentheses

Merge remote-tracking branch 'upstream/master' into removeUnnecessary…

629f588

…Parentheses

CyrusNajmabadi mentioned this pull request Apr 7, 2018

Add code-styles for controlling parentheses in a project ('coarse' approach). #26013

Merged

CyrusNajmabadi added 3 commits April 17, 2018 14:17

Merge branch 'master' into removeUnnecessaryParentheses

f203346

Merge branch 'master' into removeUnnecessaryParentheses

98b0b59

Merge branch 'master' into removeUnnecessaryParentheses

bed3b30

jinujoseph added the Need Design Review The end user experience design needs to be reviewed and approved. label Apr 28, 2018

CyrusNajmabadi closed this May 3, 2018

CyrusNajmabadi deleted the removeUnnecessaryParentheses branch May 3, 2018 19:22

jinujoseph removed the Need Design Review The end user experience design needs to be reviewed and approved. label Jul 2, 2018

Add codestyle to handle unnecessary parentheses. #21316

Add codestyle to handle unnecessary parentheses. #21316

Conversation

CyrusNajmabadi commented Aug 4, 2017 • edited Loading

CyrusNajmabadi commented Aug 4, 2017

CyrusNajmabadi commented Aug 4, 2017

CyrusNajmabadi commented Aug 4, 2017

CyrusNajmabadi commented Aug 4, 2017 • edited Loading

CyrusNajmabadi commented Aug 5, 2017

sharwell commented Aug 6, 2017 • edited Loading

CyrusNajmabadi commented Aug 7, 2017

CyrusNajmabadi commented Aug 10, 2017

CyrusNajmabadi commented Aug 10, 2017

CyrusNajmabadi commented Aug 10, 2017 • edited Loading

CyrusNajmabadi commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpoeschl commented Aug 11, 2017 • edited Loading

dpoeschl commented Aug 11, 2017 • edited Loading

sharwell commented Aug 11, 2017

sharwell commented Aug 11, 2017 • edited Loading

CyrusNajmabadi commented Aug 11, 2017

Pilchie commented Aug 11, 2017

Choose a reason for hiding this comment

sharwell commented Apr 5, 2018 • edited Loading

CyrusNajmabadi commented Apr 5, 2018 • edited Loading

CyrusNajmabadi commented Apr 5, 2018

CyrusNajmabadi commented Apr 5, 2018

CyrusNajmabadi commented Apr 5, 2018

sharwell commented Apr 5, 2018

CyrusNajmabadi commented Apr 5, 2018

Neme12 commented Apr 5, 2018

sharwell commented Apr 5, 2018

CyrusNajmabadi commented Apr 5, 2018 • edited Loading

CyrusNajmabadi commented Apr 5, 2018

sharwell commented Apr 5, 2018

CyrusNajmabadi commented Apr 5, 2018

CyrusNajmabadi commented Apr 5, 2018 • edited Loading

CyrusNajmabadi commented Apr 5, 2018 • edited Loading

kuhlenh commented Apr 5, 2018 • edited Loading

CyrusNajmabadi commented Apr 5, 2018 • edited Loading

CyrusNajmabadi commented May 3, 2018

CyrusNajmabadi commented Aug 4, 2017 •

edited

Loading

CyrusNajmabadi commented Aug 4, 2017 •

edited

Loading

sharwell commented Aug 6, 2017 •

edited

Loading

CyrusNajmabadi commented Aug 10, 2017 •

edited

Loading

dpoeschl commented Aug 11, 2017 •

edited

Loading

dpoeschl commented Aug 11, 2017 •

edited

Loading

sharwell commented Aug 11, 2017 •

edited

Loading

sharwell commented Apr 5, 2018 •

edited

Loading

CyrusNajmabadi commented Apr 5, 2018 •

edited

Loading

CyrusNajmabadi commented Apr 5, 2018 •

edited

Loading

CyrusNajmabadi commented Apr 5, 2018 •

edited

Loading

CyrusNajmabadi commented Apr 5, 2018 •

edited

Loading

kuhlenh commented Apr 5, 2018 •

edited

Loading

CyrusNajmabadi commented Apr 5, 2018 •

edited

Loading