|269 ↗||(On Diff #52798)|
Can we use .emplace_back here?
|91 ↗||(On Diff #52798)|
If it's from low to high, I'd use < instead of > to avoid confusion ;)
|99 ↗||(On Diff #52798)|
|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.
|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.
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?
|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.
|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).
Looks better now, thanks!
|163 ↗||(On Diff #54056)|
I'm not sure this will compile on VS2013.
|179 ↗||(On Diff #54056)|
|129 ↗||(On Diff #54056)|
|245 ↗||(On Diff #54056)|
nit: Name should start with a lower-case letter.
|326 ↗||(On Diff #54056)|
No need for parentheses around It->first.Checks.
Sorry for the long delay. This is getting closer. A few more comments though.
|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:
What would happen previously:
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.
|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)|
A few more instances below.
|161 ↗||(On Diff #54168)|
|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 ;)
|235 ↗||(On Diff #54168)|
Oh, I add a break statement here accidently. Remove it, and keep the caching logic here now.
|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.
Looks good with one nit.
Thank you for implementing this!
|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.