This is an archive of the discontinued LLVM Phabricator instance.

Allow per-file clang-tidy options.
ClosedPublic

Authored by alexfh on May 31 2014, 2:51 PM.

Details

Summary

This patch makes it possible for clang-tidy clients to provide
different options for different translation units. The option, which doesn't
make sense to be file-dependent, was moved to a separate ClangTidyGlobalOptions
struct. Added parsing of ClangTidyOptions.

Diff Detail

Event Timeline

alexfh updated this revision to Diff 9990.May 31 2014, 2:51 PM
alexfh retitled this revision from to Allow per-file clang-tidy options..
alexfh updated this object.
alexfh edited the test plan for this revision. (Show Details)
alexfh added a reviewer: klimek.
alexfh added a subscriber: Unknown Object (MLST).
alexfh updated this revision to Diff 9992.May 31 2014, 3:37 PM

Added a comment and an assertion, replaced a couple of |= with definitely short cirtuited versions.

klimek added inline comments.Jun 2 2014, 10:51 AM
clang-tidy/ClangTidy.cpp
206

Any reasons for not using a vector of unique_ptr's an moving that into the ASTConsumer?

214

Finder.get()?

268

Wait, this seems to have been incorrect before (| instead of logical ||), and I think we're missing parens.

clang-tidy/ClangTidyDiagnosticConsumer.h
177

Any reason not to store a std::string here? Seems fragile to store a StringRef.

clang-tidy/ClangTidyOptions.h
30–31

... for which we show warnings.

34–43

neither... nor...

37–38

I think the second sentence doesn't help. I'd delete it.

75–77

I'd vote for putting members at the end of the class.

alexfh updated this revision to Diff 10028.Jun 2 2014, 3:46 PM

Addressed review comments.

clang-tidy/ClangTidy.cpp
206

Just didn't want to touch the createChecks method. But this isn't a serious reason. Fixed.

214

It's a matter of taste. I slightly prefer &*, and &* is used two more times in this file (lines 98, 99) and in other files in clang-tidy. If you have serious reasons to prefer .get() over &*, I can change all of the usages in a separate patch, but now I would like the code to be consistent in this regard.

268

Err, it's actually incorrect now (| instead of ||). And we're missing parens (to mute the compiler warning). Before this patch it was kind of correct (except that it wasn't short circuited).

Fixed.

clang-tidy/ClangTidyDiagnosticConsumer.h
177

You're right. Changed.

clang-tidy/ClangTidyOptions.h
30–31

Done.

34–43

Done.

37–38

I'd leave it, as it's describes a special case. Without special-casing the empty list, the list would be applied always, and the empty list would mean that no warnings will pass the filter.

75–77

Done.

klimek accepted this revision.Jun 3 2014, 12:33 AM
klimek edited edge metadata.

lg

clang-tidy/ClangTidy.cpp
214

I'm probably in the "more explicit" camp here - &* will work on anything that's deref'able, .get() will only work on smart-pointer-compatible types. I like the fact that I am able to tell whether &* is just a no-op after a refactoring/replacement that hasn't been noticed (for example, after changing a reference to a pointer variable), or whether it's an intentional retrieval of the pointer.

I don't care too much, so feel free to leave it in.

clang-tidy/ClangTidyOptions.h
37–38

Interesting. My default interpretation of a filter is "for all filter, take out what it matches", which, if applied with an empty filter, doesn't change anything.
Perhaps:
If empty, no warnings will be filtered.
(or something like that)

This revision is now accepted and ready to land.Jun 3 2014, 12:33 AM
alexfh closed this revision.Jun 5 2014, 6:39 AM