[PATCH] D118927: [clang-tidy] Fix invalid fix-it for cppcoreguidelines-prefer-member-initializer

[Simon Giesecke via Phabricator via cfe-commits]

simon.giesecke added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp:537
 
+struct PR53515 {
+  int M;

Can this be renamed to something describing the test case? E.g. 
`AlreadyHasInitializer` (and the case above might be renamed to 
`AlreadyHasDefaultInitializer` to differentiate the two cases)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118927/new/

https://reviews.llvm.org/D118927

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

[Simon Giesecke via Phabricator via cfe-commits]

simon.giesecke added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3233
+
+   ``QualifierAlignment`` COULD lead to incorrect code generation.
+

This is pretty unclear, for a number of reasons:
* First, this probably only refers to a setting other than `QAS_Leave`?
* Second, "lead to incorrect code generation" seems to skip a step. In the 
first place, this seems to imply that a setting other than `QAS_Leave` might 
change the semantics of the source code.
* Third, it's not clear to me when this would happen, and how likely that is. 
Does this mean "Non-default settings are experimental, and you shouldn't use 
this in production?" or rather "Under rare circumstances that are unlikely to 
happen in real code, a non-default setting might change semantics." At the 
minimum, could this give some example(s) when this happens?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

[Simon Giesecke via Phabricator via cfe-commits]

simon.giesecke added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3233
+
+   ``QualifierAlignment`` COULD lead to incorrect code generation.
+

MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > simon.giesecke wrote:
> > > This is pretty unclear, for a number of reasons:
> > > * First, this probably only refers to a setting other than `QAS_Leave`?
> > > * Second, "lead to incorrect code generation" seems to skip a step. In 
> > > the first place, this seems to imply that a setting other than 
> > > `QAS_Leave` might change the semantics of the source code.
> > > * Third, it's not clear to me when this would happen, and how likely that 
> > > is. Does this mean "Non-default settings are experimental, and you 
> > > shouldn't use this in production?" or rather "Under rare circumstances 
> > > that are unlikely to happen in real code, a non-default setting might 
> > > change semantics." At the minimum, could this give some example(s) when 
> > > this happens?
> > * Yes.
> > * Yes, it might change the semantics, that was the content of a huge 
> > discussion here in the review and on the mailing list. Consensus was found 
> > that non whitespace changes are acceptable with a big warning and off by 
> > default.
> > * The latter, if we would have an example at hand in which cases it 
> > wouldn't work, we would fix that and not give it as an example. So to the 
> > best of our knowledge it does not break anything.
> > 
> > The warning text however might be improved.
> Agreed its tough to give a meaningful example that actually currently breaks 
> code, but the example I was given was
> 
> ```
> #define INTPTR int *
> 
> const INTPTR a;
> ```
> 
> In this sense moving const changes this from
> 
> ```
> const int * a;
> ```
> to 
> 
> ```
> int * const a;
> ```
> 
> For this we overcome this by NOT supporting "UPPERCASE" identifiers incase 
> they are macros, we have plans to add support for identifying "type 
> identifiers"
> 
> I guess if someone does
> 
> ```
> #define intptr int *
> ```
> 
> Then this could go still go wrong, however I like to think that these 
> examples are on the "edge" of good code. (should we pander to what could be 
> considered bad style in the first place?)
> 
> The warning was a concession to those who felt its should be highlighted as 
> an option that could change behaviour (like SortIncludes was famously 
> identified),  and as @HazardyKnusperkeks  say, we are looking for people to 
> tell us where this breaks so we can try and fix it. (macros are already an 
> issue for clang-format the solution for which is clang-format off/on)
> 
> I personally feel the no need to elaborate further on the warning at this 
> time, but I'm happy to support someone if they feel they want to extend it. 
> 
> If you are concerned my advice is don't use this option. But clang-format can 
> be used in an advisor capacity (with -n or --dry-run) and this can be used to 
> notify cases of incorrect qualifier ordering without actually using it to 
> change the code.
Thanks a lot for the explanation. Sorry I hadn't read through the entire review 
discussion. I saw the comment in the generated documentation and then dug up 
this patch that introduced it.

I see how macros might break this (I think I ran into a similar problem when 
trying to fix the const alignment manually using some `sed` machinery, though 
luckily it broke the build and didn't end up in bad code generation). It would 
be good to understand if there are any cases not involving macros whose 
semantics are modified. Identifying macros via the naming convention can't be 
reliable. Even if one did that consistently in their own code base, chances are 
high that library headers don't follow the same conventions. And yes, probably 
cases where a qualifier is added via a macro is not good style anyway. I 
replaced that by `std::add_const_t` in the mentioned case. But the use cases of 
clang-format are probably not confined to code written in a good style. 

I think it's good to extend the warning a bit, otherwise anyone reading it 
would need to dig up this patch and read through the review discussion to judge 
it. I'll leave a comment on D110801 as well.

> If you are concerned my advice is don't use this option. 

Well, I think if it works "reliably", it's very useful. I was searching for a 
way to harmonize the const/qualifier alignment style, and I first thought that 
would be clang-tidy land, and then found this clang-format patch. And I guess, 
maybe that's exactly the gap that leads to this problem: In clang-tidy, there 
would be the extra semantic information (we know what's a macro or type etc.) 
that would allow to prevent any semantic changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764

___
cfe-commits mailing list
cfe-commits@lists.llv

[PATCH] D110801: [clang-format] [docs] [NFC] improve clarity in the QualifierAlignment warning

[Simon Giesecke via Phabricator via cfe-commits]

simon.giesecke added inline comments.



Comment at: clang/include/clang/Format/Format.h:1902
+  ///  Setting ``QualifierAlignment``  to something other than `Leave`, COULD
+  ///  lead to incorrect code generation due to a lack of semantic information
+  ///  especially in the presense of macros, care should be take to review code

I don't think `incorrect code generation` is the decisive point here (or at 
least the wording is confusing if "code generation" refers to the output of 
clang-format rather than, what I would understand, the binary code generated by 
the compiler). Rather it's that the semantics of the source code might change. 
Whether that causes syntax errors, bad code generation, or remains without any 
visible effect is probably impossible to judge here.



Comment at: clang/include/clang/Format/Format.h:1903
+  ///  lead to incorrect code generation due to a lack of semantic information
+  ///  especially in the presense of macros, care should be take to review code
+  ///  changes made by this option.

curdeius wrote:
> 
As discussed around https://reviews.llvm.org/D69764#3032727, I see how macros 
could break this.

But what exactly does "especially" mean here, with respect to cases not 
involving macros?

Does it mean:
* We have some indication that this might break even without macros.
* We have no such indication but want to play safe until this has been tested 
in the wild.
* We can never know for sure...

At least in the last case, I'd suggest to remove the word "especially". That's 
true for all options of clang-format...

Similarly in the second case. That's true for all newly introduced options of 
clang-format.

In the first case, it makes sense to keep the word. But some more specific 
indication of the potential issues with and without macros seem to be helpful.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110801/new/

https://reviews.llvm.org/D110801

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113429: [clang-tidy] Use `hasCanonicalType()` matcher in `bugprone-unused-raii` check

[Simon Giesecke via Phabricator via cfe-commits]

simon.giesecke accepted this revision.
simon.giesecke added a comment.
This revision is now accepted and ready to land.

LGTM, thanks a lot!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113429/new/

https://reviews.llvm.org/D113429

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115106: [clang-tidy] Fix `readability-static-accessed-through-instance` false negative for static methods

[Simon Giesecke via Phabricator via cfe-commits]

simon.giesecke added a comment.

Thanks a lot for addressing this issue! I am just trying it on our codebase.

> The problem actually has nothing to do with the out-of-line definition being 
> inline; the problem is that hasDeclaration() of the memberExpr() will match 
> the out-of-line definition, which obviously isn't marked static, so 
> isStaticStorageClass() won't match.

Hm, an out-of-line definition *cannot* have the `static` keyword. I wonder if 
it's actually a bug (in the AST? or just the matcher?) that 
`isStaticStorageClass` doesn't match here? I guess that other checks that use 
`isStaticStorageClass` might be affected by this too?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115106/new/

https://reviews.llvm.org/D115106

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115106: [clang-tidy] Fix `readability-static-accessed-through-instance` false negative for static methods

[Simon Giesecke via Phabricator via cfe-commits]

simon.giesecke added a comment.

In D115106#3180439 , @aaron.ballman 
wrote:

> In D115106#3172949 , 
> @simon.giesecke wrote:
>
>> Thanks a lot for addressing this issue! I am just trying it on our codebase.
>>
>>> The problem actually has nothing to do with the out-of-line definition 
>>> being inline; the problem is that hasDeclaration() of the memberExpr() will 
>>> match the out-of-line definition, which obviously isn't marked static, so 
>>> isStaticStorageClass() won't match.
>>
>> Hm, an out-of-line definition *cannot* have the `static` keyword. I wonder 
>> if it's actually a bug (in the AST? or just the matcher?) that 
>> `isStaticStorageClass` doesn't match here? I guess that other checks that 
>> use `isStaticStorageClass` might be affected by this too?
>
> `isStaticStorageClass()` calls through to `FunctionDecl::getStorageClass()` 
> which reports the storage as written on that declaration in source. So this 
> isn't a bug in the AST, it's a more of a misunderstanding of whether we're 
> querying the storage duration/linkage for the method or whether we're 
> querying whether it was written with the static keyword.

I see, makes sense.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115106/new/

https://reviews.llvm.org/D115106

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115106: [clang-tidy] Fix `readability-static-accessed-through-instance` false negative for static methods

[Simon Giesecke via Phabricator via cfe-commits]

simon.giesecke added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6017
+/// cxxMethodDecl(isStatic()) matches A::foo() but not A::bar()
+AST_MATCHER(CXXMethodDecl, isStatic) { return Node.isStatic(); }
+

aaron.ballman wrote:
> I would prefer we not expose this AST matcher as a public one. There's 
> sufficient confusion around "as written in source" vs "computed" vs "linkage" 
> in this area that I think we should be careful when introducing matchers 
> around storage classes and storage durations. Then again, the water is 
> already pretty muddy here, so maybe this ship sailed...
> 
> Other potential solutions to this issue would be to expose an AST matcher to 
> traverse to the canonical decl or only matches the canonical decl, then we 
> could use that to wrap the existing call to `isStaticStorageClass()`. (Of 
> course, we could make this matcher local to just that source file, as well.)
> 
> Thoughts?
I think if we have a `isStaticStorageClass` matcher, then we can also have 
`isStatic` matcher.

But maybe we shouldn't have either of those in the public header, but maybe add 
a different one as you suggested, under a suitable name with as little 
ambiguity as possible.

And maybe keep the `isStaticStorageClass` matcher but rename it to reduce the 
chance of misunderstanding?

Then, as mentioned in my original top-level comment, I can imagine that there 
are more uses of the `isStaticStorageClass` matcher in clang-tidy rules (and 
maybe elsewhere) that actually should use the new matcher.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115106/new/

https://reviews.llvm.org/D115106

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93543: clang-tidy: Add `-use-color` for `run-clang-tidy.py` in order to make it sane with `clang-tidy` coloring argument

[Simon Giesecke via Phabricator via cfe-commits]

simon.giesecke added a comment.

I think this would be a useful change. In the current state, one needs to 
modify the script to run this in a context where coloring is not supported.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93543/new/

https://reviews.llvm.org/D93543

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits