[PATCH] D16113: [clang-tdiy] Add header file extension configuration support.

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+                                 llvm::StringRef HeaderFileExtensions);
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > hokein wrote:
> > > > aaron.ballman wrote:
> > > > > alexfh wrote:
> > > > > > This function doesn't belong here. I'm also not sure we need this function at all. First, it's ineffective to split the string containing the list of extensions each time. Second, if we store a set of extensions, then we can just search for the actual file extension in this set.
> > > > > endsWithHeaderFileExtension instead? However, given that uses of this all start with a SourceLocation, I wonder if that makes for a cleaner API: isLocInHeaderFile(SourceLocation, <Extensions>);
> > > > > 
> > > > > Also, how does this work if I want to include an extension-less file as the header file "extension?" It would be plausible if the extensions were passed in as a list, but as it stands it doesn't seem possible without weird conventions like leaving a blank in the list (e.g., `.h,,.hpp`), which seems error-prone.
> > > > > 
> > > > > Also, I'm not certain what I can pass in. The documentation should be updated to state whether these extensions are intended to include the ".".
> > > > > endsWithHeaderFileExtension instead? However, given that uses of this all start with a SourceLocation, I wonder if that makes for a cleaner API: isLocInHeaderFile(SourceLocation, <Extensions>);
> > > > 
> > > > Using `SourceLocation` only is not enough to retrieve the belonging file name (we need `SourceManager` too).
> > > > 
> > > > >Also, how does this work if I want to include an extension-less file as the header file "extension?" It would be plausible if the extensions were passed in as a list, but as it stands it doesn't seem possible without weird conventions like leaving a blank in the list (e.g., .h,,.hpp), which seems error-prone.
> > > > 
> > > > Yeah, for extensions-less header file, you can pass the string like `.h,,.hpp`, which is a bit of weird. Do you have a better idea here? Passing a string into `header-file-extensions` seems the most reasonable choice. 
> > > > 
> > > `isLocInHeaderFile(SourceLocation, ...)` is a nice idea, but we'd need to be more specific: either `isExpansionLocInHeaderFile(SourceLoc, ...)` or `isSpellingLocInHeaderFile(SourceLoc, ...)` (or both).
> > > Yeah, for extensions-less header file, you can pass the string like .h,,.hpp, which is a bit of weird. Do you have a better idea here? Passing a string into header-file-extensions seems the most reasonable choice.
> > 
> > I thought those user configurations from the command line were in YAML or JSON format, those both have the notion of lists, don't they? I would imagine this would take a SmallVectorImpl<StringRef/std::string> for the list of extensions.
> > isLocInHeaderFile(SourceLocation, ...) is a nice idea, but we'd need to be more specific: either isExpansionLocInHeaderFile(SourceLoc, ...) or isSpellingLocInHeaderFile(SourceLoc, ...) (or both).
> 
> That's true, and I would think both are reasonable to add. I rather prefer that as an API instead of passing around file names as strings, personally.
User configurations are stored in YAML, however, CheckOptions is a map of strings to strings, so we can't use YAML lists to represent lists of extensions.
I personally see nothing wrong with `",.h,.hh,.hpp"` for example, to represent the list of extensions (<empty> being the first one makes it somewhat stand out and provided there's a good documentation, this shouldn't be too confusing).


Repository:
  rL LLVM

http://reviews.llvm.org/D16113