[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:
> alexfh wrote:
> > 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).
> > 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).
> 
> I find it to be more clever than intuitive. If it were not user-facing, I would be less concerned. I don't think users should have to read documentation to figure out the *syntax* of how to pass options if we can at all avoid it. ;-)
> 
> Regardless, I would like to separate the two concepts -- there's the way we expose the option to the users, and there's our internal APIs that we call. I don't think the internal API has to take such an awkward thing directly just because the user-facing option has to be that way currently.
This aligns with what I wrote in my first comment on this:

> 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.

I still think the function doesn't belong here. And we also shouldn't split the string each time we call it. So I suggest adding these functions to clang-tidy/utils/:

```
bool isExpansionLocInHeader(const? SourceManager &SM, SourceLocation Loc, const StringSet &HeaderFileExtensions);
bool isSpellingLocInHeader(const? SourceManager &SM, SourceLocation Loc, const StringSet &HeaderFileExtensions);
StringSet splitCommaSeparatedSet(StringRef S); // Or something similar.
```


Repository:
  rL LLVM

http://reviews.llvm.org/D16113