[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Fully agree. Such an opt-in feature shouldn't break things. I'd be happy to 
implement it if people think it's a good idea! Would need some hand-holding 
though, never touched the LLVM codebase before :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566

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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I think this should be as simple as having an opt-in "DisableAllAliaseeChecks" 
option in `.clang-tidy`, that force-disables all the checks that are actually 
aliases and not proper checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566

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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.
Herald added a subscriber: jeroen.dobbelaere.

Hi!

I've been following this issue about aliases in clang-tidy through different 
bug reports and commit attempts. I see that the author abandoned this commit 
without an explanation. What's the reason? Where can I follow the continuation 
of this discussion, is there a email list or something?

I believe making sure that clang-tidy only runs once instance of the checks is 
important, it can dramatically reduce the runtime. This can have a significant 
impact in CI working with large codebases. It also doesn't make sense that 
alias checks aren't identical and don't detect the same things. As it has been 
mentioned above, users should not carry the burden of knowing and maintaining 
what is aliasing what at any point in time, that's an internal implementation 
detail of clang-tidy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566

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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Martin G via Phabricator via cfe-commits]

MartinG added a comment.

In D72566#1815917 , @aaron.ballman 
wrote:

> I'm not certain I agree. Many of the aliases have different sets of default 
> checking options from the primary check.

And that's precisely the problem. Aliases should be just that: aliases. If two 
checks check for the same thing but have different options, they should be 
merged into one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566

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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72566#1816109 , @lebedev.ri wrote:

> I think the fact that this is a fourth (?) different incarnation of a patch
>  trying to solve the same *major* *ugly* problem, it may be an evidence that
>  perhaps this problem should not be approached from the 'let's hack it 
> together'
>  approach, but with a concrete plan sent as an RFC to cfe-dev :/


+1

This is a pretty big part of the user interface for clang-tidy, so I think we 
need community buy-in on how to proceed. It's not that we think the current 
interface is perfect, it's more that there are complexities we need to better 
understand before deciding on a solution. I'd rather get a big picture idea of 
the design for aliases, how they interact with the command line, documentation, 
diagnostics, etc to make sure we're hitting all of the important bits. Also, we 
should make sure that whatever we do is at least someone comparable with the 
clang static analyzer (I don't know if they have aliases there or not) so that 
we don't design two totally different approaches to solving the same problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566



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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I think the fact that this is a fourth (?) different incarnation of a patch
trying to solve the same *major* *ugly* problem, it may be an evidence that
perhaps this problem should not be approached from the 'let's hack it together'
approach, but with a concrete plan sent as an RFC to cfe-dev :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566



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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Another potential solution is leave is as is by default. Then emit a warning if 
a check is ran twice. The warning could be suppressed by passing a option to 
say what behaviour you want?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566



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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

How does this sound? 
I'll put in a config option in there to disable the new alias behaviour.
Personally I'd say default to the new behaviour. Then if an organisation
relies on the old behaviour then can revert back with a command line option
or .clang-tidy file edit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566



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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72566#1815913 , @njames93 wrote:

> In D72566#1815904 , @lebedev.ri 
> wrote:
>
> > I agree that the current alias situation is not ideal, though i'm not sure
> >  how much we can fix it since it crosses user interface boundary
> >  (i.e. what fixes won't lead to some regressions from the previous expected 
> > behavior)
>
>
>   If you're expecting a test to run twice usually something has gone wrong


I'm not certain I agree. Many of the aliases have different sets of default 
checking options from the primary check. Running the "same" check twice in 
these cases produces different output. How do you intend to handle situations 
like that? If the answer is "disable the aliases", then this has the potential 
to lose valuable coverage for some folks who are asking for all checks to be 
enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566



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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D72566#1815913 , @njames93 wrote:

> In D72566#1815904 , @lebedev.ri 
> wrote:
>
> > I agree that the current alias situation is not ideal, though i'm not sure
> >  how much we can fix it since it crosses user interface boundary
> >  (i.e. what fixes won't lead to some regressions from the previous expected 
> > behavior)
>
>
>   If you're expecting a test to run twice usually something has gone wrong


That is not what i'm talking about.
I'm mainly worried about checks with custom config options:
It is likely that said options were specified only for one check,
not all of it's aliases too. So now the fact whether or not
these options will be used will depend on the registering order of checks.

>> To be noted, this will make adding of new aliases in non-up-stream modules 
>> more involved,
>>  since now the aliases will no longer be confined to the new module, but to 
>> the original modules.
> 
> That's how it currently is for adding new alias definitions. 
>  When you create an alias definition now you need the implementation files 
> available.
>  However this approach doesn't need to modify the original module and appears 
> mostly transparent to it.
>  Its also made in such a way that if you set a check as aliasing to a now 
> existent name, it would just give you another way to invoke said check
> 
>> If we proceed as-is, i think we also need one more config switch 
>> "CheckAliasesAreEnabled = 1".
>>  And we should completely ignore aliases unless it's enabled.
> 
> That does sound like something I can get behind
> 
> There is one major nit I can think of and that is if you create an alias to 
> the wrong check it would overwrite said check. I may look into adjusting that




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566



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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D72566#1815904 , @lebedev.ri wrote:

> I agree that the current alias situation is not ideal, though i'm not sure
>  how much we can fix it since it crosses user interface boundary
>  (i.e. what fixes won't lead to some regressions from the previous expected 
> behavior)


If you're expecting a test to run twice usually something has gone wrong

> To be noted, this will make adding of new aliases in non-up-stream modules 
> more involved,
>  since now the aliases will no longer be confined to the new module, but to 
> the original modules.

That's how it currently is for adding new alias definitions. 
When you create an alias definition now you need the implementation files 
available.
However this approach doesn't need to modify the original module and appears 
mostly transparent to it.
Its also made in such a way that if you set a check as aliasing to a now 
existent name, it would just give you another way to invoke said check

> If we proceed as-is, i think we also need one more config switch 
> "CheckAliasesAreEnabled = 1".
>  And we should completely ignore aliases unless it's enabled.

That does sound like something I can get behind

There is one major nit I can think of and that is if you create an alias to the 
wrong check it would overwrite said check. I may look into adjusting that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566



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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I agree that the current alias situation is not ideal, though i'm not sure
how much we can fix it since it crosses user interface boundary
(i.e. what fixes won't lead to some regressions from the previous expected 
behavior)

To be noted, this will make adding of new aliases in non-up-stream modules more 
involved,
since now the aliases will no longer be confined to the new module, but to the 
original modules.

Also, there was previously some attempts at this:
https://lists.llvm.org/pipermail/cfe-dev/2017-September/055589.html
D25659 
D38171 
???

If we proceed as-is, i think we also need one more config switch 
"CheckAliasesAreEnabled = 1".
And we should completely ignore aliases unless it's enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566



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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

should warning be printed with all the enabled checks, or just the primary name?

  warning: statement should be inside braces 
[readability-braces-around-statements]

vs

  warning: statement should be inside braces 
[readability-braces-around-statements, hicpp-braces-around-statements]
  warning: statement should be inside braces 
[readability-braces-around-statements], [hicpp-braces-around-statements]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566



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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61774 tests passed, 0 failed 
and 780 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566



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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: aaron.ballman, alexfh, JonasToth, hokein.
njames93 added projects: clang-tools-extra, clang.
Herald added a subscriber: xazax.hun.

Clang-tidy has had an on growing issue with blindly adding all checks from a 
module using the `-checks=somemodule-* ` option. If somemodule alias' another 
enabled check. the check will get ran twice giving duplicated warning and fix 
options. This can lead to fixes overlapping preventing them from being applied 
or worse still applying twice resulting in malformed code. There are other 
issues with ConfigOptions being different in the 2 different instances so one 
instance of the check may be running a different set of options to the other.

This patch solves these issue by making clang-tidy only run 1 instance of an 
aliased check even if it has been turned on with multiple names. Warning 
messages are appended with the original check name if it has been enabled(via 
cmd line, or config file). If the original check hasn't been enabled then the 
name will be the first enabled alias that was registered.

Config options are also handled in a similar fashion, see example

  [{ key: MyAliasedName.Option1, value: 0 }.
  { key: OriginalCheckName.Option1, value: 1 }]

If the check `original-check-name` was turned on, then `Option1` will be parsed 
as being `1`, otherwise it ignores `OriginalCheckName.Option1` key and falls 
back to the first enabled alias check that declares `Option1`

To declare a check as an alias when you register then check, just add the check 
this alias' to as a second parameter

  CheckFactories.registerCheck(
  "hicpp-braces-around-statements");
  // Update to
  CheckFactories.registerCheck(
  "hicpp-braces-around-statements", "readability-braces-around-statements");

I haven't added in all the alias declarations in just yet (If you want me to 
just ask), nor have I added test cases. However once all the alias declarations 
are added in, the tests fall nicely in line with the current test 
infrastructure.
Adding this line to the readability-braces-around-statements checks causes no 
errors, no multiple fix attempts or warning messages.

  // RUN: %check_clang_tidy %s 
readability-braces-around-statements,hicpp-readability-braces-around-statements 
%t

Likewise changing it to

  // RUN: %check_clang_tidy %s hicpp-readability-braces-around-statements %t

Also runs without a hitch (obviously warning messages are appended with 
`[hicpp-readability-braces-around-statements]` instead of 
`[readability-braces-around-statements]` as above.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72566

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/ClangTidyCheck.h
  clang-tools-extra/clang-tidy/ClangTidyModule.cpp
  clang-tools-extra/clang-tidy/ClangTidyModule.h
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,6 +67,9 @@
 Improvements to clang-tidy
 --
 
+- Clang tidy is now aware of alias checks and will only run one instance of
+  an alias check that's enabled by different names.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/ClangTidyModule.h
===
--- clang-tools-extra/clang-tidy/ClangTidyModule.h
+++ clang-tools-extra/clang-tidy/ClangTidyModule.h
@@ -11,6 +11,7 @@
 
 #include "ClangTidy.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
 #include 
 #include 
 #include 
@@ -34,6 +35,13 @@
   /// For all checks that have default constructors, use \c registerCheck.
   void registerCheckFactory(StringRef Name, CheckFactory Factory);
 
+  /// Registers check \p Factory with name \p Name that is an alias to
+  /// another check called \p AliasToName.
+  ///
+  /// For all checks that have default constructors, use \c registerCheck.
+  void registerCheckFactory(StringRef Name, StringRef AliasToName,
+CheckFactory Factory);
+
   /// Registers the \c CheckType with the name \p Name.
   ///
   /// This method should be used for all \c ClangTidyChecks that don't require
@@ -62,6 +70,16 @@
  });
   }
 
+  /// Registers the \c CheckType with the name \p Name that is an alias to
+  /// another check called \p AliasToName.
+  template 
+  void registerCheck(StringRef CheckName, StringRef AliasToName) {
+registerCheckFactory(CheckName, AliasToName,
+ [](StringRef Name, ClangTidyContext *Context) {
+   return std::make_unique(Name, Context);
+ });
+  }
+
   /// Create instances of checks that are enabled.
   std::vector>
   createChecks(ClangTidyContext *Context);
@@