This is an archive of the discontinued LLVM Phabricator instance.

[ClangTidy] Add an 'explain-checks' option to diagnose where each checks comes from.
ClosedPublic

Authored by hokein on Apr 1 2016, 6:26 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 52361.Apr 1 2016, 6:26 AM
hokein updated this revision to Diff 52363.
hokein retitled this revision from to [ClangTidy] Add an 'explain-checks' option to diagnose where each checks comes from..
hokein updated this object.

Update

hokein set the repository for this revision to rL LLVM.
hokein added a subscriber: cfe-commits.
alexfh edited edge metadata.Apr 1 2016, 8:07 AM

Nice! I like the idea, but we need to figure out a bunch of details (see the comments inline).

clang-tidy/ClangTidyOptions.cpp
264 ↗(On Diff #52363)

ConfigFile.str() should also work.

clang-tidy/tool/ClangTidyMain.cpp
131 ↗(On Diff #52363)

I would expect something different from an option named '-explain-checks', e.g. some kind of a short documentation on each check or the like. Something like explain-config or describe-config would be closer to what the option does, I think.

132 ↗(On Diff #52363)

How about for each enabled check explains, where it is enabled, i.e. in clang-tidy binary, command line or a specific configuration file?

265 ↗(On Diff #52363)

s/ClangTidy/clang-tidy/

278 ↗(On Diff #52363)

s/argument 'checks'/option '-checks'/

294 ↗(On Diff #52363)

s/argument 'config'/option '-config'/

329 ↗(On Diff #52363)

I'm not sure I understand how this should work in case a check is:

  1. enabled in binary (e.g. -*,a)
  2. disabled in the config file (e.g. -a)
  3. enabled again on the command line (e.g. a)

clang-tidy should say 'command line' in this case, but i'm not sure what your implementation will do, since it doesn't seem to keep the order of the configurations.

331 ↗(On Diff #52363)

nit: Filter

You could also write if (GlobList(CheckSource.first).contains(Check)) {

333 ↗(On Diff #52363)

s/comes from/is enabled in the/

alexfh requested changes to this revision.Apr 1 2016, 8:08 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 1 2016, 8:08 AM
hokein updated this revision to Diff 52549.Apr 4 2016, 6:55 AM
hokein edited edge metadata.
hokein marked 7 inline comments as done.

Update.

hokein updated this revision to Diff 52550.Apr 4 2016, 7:05 AM
hokein marked an inline comment as done.
hokein edited edge metadata.

Remove redundant code.

hokein marked an inline comment as done.Apr 4 2016, 7:10 AM
hokein added inline comments.
clang-tidy/tool/ClangTidyMain.cpp
329 ↗(On Diff #52550)

I updated the patch and made sure the behavior keep the order of configuration.

But if the case like:

  1. enabled in binary
  2. enabled in config file
  3. enabled in command-line option -checks

Only command-line option -checks will be diagnosed, since the processor order of configuration is binary -> config file -> command-line option -checks.

alexfh added inline comments.Apr 4 2016, 8:13 AM
clang-tidy/tool/ClangTidyMain.cpp
329 ↗(On Diff #52550)

As discussed offline, the order of iteration matters here. And it looks like we should look at the complete Checks options without splitting them to components (in order to handle negative patterns).

alexfh requested changes to this revision.Apr 4 2016, 8:14 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 4 2016, 8:14 AM
hokein updated this revision to Diff 52797.Apr 6 2016, 7:50 AM
hokein edited edge metadata.
hokein marked an inline comment as done.

Correct the processing orders.

hokein updated this revision to Diff 52798.Apr 6 2016, 7:54 AM
hokein edited edge metadata.

More comments.

hokein marked an inline comment as done.Apr 6 2016, 7:58 AM
alexfh added inline comments.Apr 7 2016, 10:55 AM
clang-tidy/ClangTidyOptions.cpp
269 ↗(On Diff #52798)

Can we use .emplace_back here?

clang-tidy/ClangTidyOptions.h
91 ↗(On Diff #52798)

If it's from low to high, I'd use < instead of > to avoid confusion ;)

99 ↗(On Diff #52798)
  1. Use /// for doxygen comments.
  2. Insert an empty comment line after this line to mark the end of the \brief block.
  3. Mark the list items with *.
104 ↗(On Diff #52798)

Can we store a single vector of string pairs instead of three different vectors? The second item of the pair already says enough about the source. And this being an array doesn't seem to be useful.

clang-tidy/tool/ClangTidyMain.cpp
281 ↗(On Diff #52798)

Please pull this string literal to a constant: it's used more than once in the code. Maybe also pull the "clang-tidy binary" string too, for consistency.

alexfh requested changes to this revision.Apr 7 2016, 10:55 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 7 2016, 10:55 AM
alexfh added inline comments.Apr 7 2016, 10:56 AM
clang-tidy/tool/ClangTidyMain.cpp
281 ↗(On Diff #52798)

Err, the strings are actually different, but I'd still make all three of them constants.

hokein updated this revision to Diff 53203.Apr 11 2016, 1:13 AM
hokein edited edge metadata.

Use one vector storing checks details instead of three.

hokein marked 5 inline comments as done.Apr 11 2016, 1:18 AM

I've just realized that the approach is artificially limited to just keeping track of the origin of the Checks option, while it could be easily extended to track the origin of all configuration items. What if instead of std::vector<StringPair> CheckSources; we introduce std::vector<std::pair<std::string, ClangTidyOptions>> Sources;? Then we could be able to find where a certain check option or an extra argument was introduced, for example. WDYT?

clang-tidy/tool/ClangTidyMain.cpp
188 ↗(On Diff #53203)

One problem with this way of defining string constants is that they are not constants. CheckSourceTypeDefaultBinary = "oops"; will work just fine. Instead, I prefer to use constexpr char XXX[] = "..."; construct.

alexfh requested changes to this revision.Apr 11 2016, 3:48 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 11 2016, 3:48 AM
hokein updated this revision to Diff 53372.Apr 12 2016, 1:53 AM
hokein edited edge metadata.

Keep track all the fields of ClangTidyOptions.

hokein marked an inline comment as done.Apr 12 2016, 1:58 AM

I've just realized that the approach is artificially limited to just keeping track of the origin of the Checks option, while it could be easily extended to track the origin of all configuration items. What if instead of std::vector<StringPair> CheckSources; we introduce std::vector<std::pair<std::string, ClangTidyOptions>> Sources;? Then we could be able to find where a certain check option or an extra argument was introduced, for example. WDYT?

Update the patch according to our offline discussion. One different thing is that we can't assume that Other in merge(CTO& Other) has empty HigherPriorityOptions because some usages break this assumption.

hokein updated this revision to Diff 53373.Apr 12 2016, 2:03 AM
hokein edited edge metadata.

Don't modify unrelevant code.

alexfh requested changes to this revision.Apr 15 2016, 5:03 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/ClangTidyOptions.h
99 ↗(On Diff #53373)

As per offline discussion, there might be a more elegant and robust solution to this. For example, extending the ClangTidyOptionsProvider interface with a virtual getRawOptions method (ideas for a better name are welcome) returning an ordered list of ClangTidyOption objects accompanied by textual descriptions of their origin (defaults/configuration file name/command line option name). The virtual ClangTidyOptions getOptions(llvm::StringRef FileName); method can then be made non-virtual and serve as an interface for option consumers only (it would just merge all options returned by the former method).

This revision now requires changes to proceed.Apr 15 2016, 5:03 AM
hokein updated this revision to Diff 54042.Apr 18 2016, 3:40 AM
hokein edited edge metadata.

Use a more elegant solution to track ClangTidyOptions.

hokein updated this revision to Diff 54043.Apr 18 2016, 3:43 AM
hokein edited edge metadata.

Update.

hokein marked an inline comment as done.Apr 18 2016, 3:44 AM
hokein updated this revision to Diff 54056.Apr 18 2016, 6:29 AM

Export SourceType.

Looks better now, thanks!

clang-tidy/ClangTidyOptions.cpp
163 ↗(On Diff #54056)

I'm not sure this will compile on VS2013.

179 ↗(On Diff #54056)

Maybe emplace_back?

clang-tidy/ClangTidyOptions.h
129 ↗(On Diff #54056)

Clang-format, please.

245 ↗(On Diff #54056)

nit: Name should start with a lower-case letter.

clang-tidy/tool/ClangTidyMain.cpp
326 ↗(On Diff #54056)

No need for parentheses around It->first.Checks.

hokein updated this revision to Diff 54168.Apr 19 2016, 2:01 AM

Address code review comments.

hokein marked 5 inline comments as done.Apr 19 2016, 2:02 AM
hokein added inline comments.
clang-tidy/ClangTidyOptions.cpp
163 ↗(On Diff #54168)

Yeah, you are right. VS2013 doesn't work with vector brace-initialization.

hokein updated this revision to Diff 54996.Apr 26 2016, 6:04 AM
hokein marked an inline comment as done.
  • VS2013 doesn't support constexpr.
  • Don't make test read external .clang-tidy file.

Sorry for the long delay. This is getting closer. A few more comments though.

clang-tidy/ClangTidyOptions.cpp
224 ↗(On Diff #54168)

s/ = \s*OptionsSource//

235 ↗(On Diff #54168)

This seems to be changing the caching logic. Consider this directory structure:

a/
   .clang-tidy
   b/
      c/

And consequtive getRawOptions calls with:

  1. "a/some_file"
  2. "a/b/c/some_file"
  3. "a/b/some_file".

What would happen previously:

  1. after call 1 CachedOptions would contain an entry for "a"
  2. call 2 would find an entry for "a" and copy it for "a/b" and "a/b/c"
  3. call 3 would just use the cache entry for "a/b"

Now step 2 doesn't copy the cache entry to "a/b" and "a/b/c".

Is there any specific reason to change this? This is benign given that the lookups happen in memory, but then the code needs to be consistent and avoid replicating cache entries to intermediate directories in all cases.

clang-tidy/ClangTidyOptions.h
102 ↗(On Diff #54168)

As we figured out with another patch, constexpr is not supported by VS2013. Sorry for pushing you in a wrong direction.

115 ↗(On Diff #54168)

s/from low to top/in order of increasing priority/

117 ↗(On Diff #54168)

This item should make it clear how a config file relates to the '-config' option in terms of priority. The easiest way to make the order clear is probably to split the item in two (and update the "3 types of the sources" above).

127 ↗(On Diff #54168)

Let's move the implementation to the .cpp file. It's only used by a small fraction of translation units including this header, and it doesn't seem to be performance-critical to make this method inline.

145 ↗(On Diff #54168)

clang-format?

A few more instances below.

161 ↗(On Diff #54168)

clang-format?

clang-tidy/tool/ClangTidyMain.cpp
320 ↗(On Diff #54168)

"a more elegant way" seems to suggest that currently this already happens, but in a not elegant fashion. As far as I understand, we're not doing this at all ;)

alexfh requested changes to this revision.Apr 26 2016, 6:04 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 26 2016, 6:04 AM
hokein updated this revision to Diff 55001.Apr 26 2016, 6:45 AM
hokein edited edge metadata.
hokein marked 8 inline comments as done.

Address comments.

hokein added inline comments.Apr 26 2016, 6:47 AM
clang-tidy/ClangTidyOptions.cpp
235 ↗(On Diff #54168)

Oh, I add a break statement here accidently. Remove it, and keep the caching logic here now.

clang-tidy/ClangTidyOptions.h
115 ↗(On Diff #54996)

Explaining the priority of config option and config file is't reasonable here since clang-tidy only takes one of them. If the config option is specified, clang-tidy just ignores the config file.

hokein marked an inline comment as done.Apr 26 2016, 6:48 AM
alexfh accepted this revision.Apr 26 2016, 6:56 AM
alexfh edited edge metadata.

Looks good with one nit.

Thank you for implementing this!

clang-tidy/tool/ClangTidyMain.cpp
320 ↗(On Diff #55001)

Sorry for being not clear. We still need to find a way to display the source of other components of the configuration. So the FIXME was fine, just the wording was confusing. Please add the FIXME back, but just remove the "more elegant" part.

This revision is now accepted and ready to land.Apr 26 2016, 6:56 AM

One more nit.

clang-tidy/ClangTidyOptions.h
233 ↗(On Diff #55001)

clang-format?

Consider making git clang-format handle it for you ;)

hokein updated this revision to Diff 55170.Apr 27 2016, 1:23 AM
hokein edited edge metadata.

Fix remaining nits.

hokein marked 2 inline comments as done.Apr 27 2016, 1:23 AM
hokein added inline comments.
clang-tidy/ClangTidyOptions.h
233 ↗(On Diff #55001)

Done. Format the whole patch now.

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
Via Conduit