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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+                                 llvm::StringRef HeaderFileExtensions);
----------------
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.

================
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+                                 llvm::StringRef HeaderFileExtensions);
----------------
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.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:78
@@ +77,3 @@
+static cl::opt<std::string>
+HeaderFileExtensions("header-file-extensions",
+                     cl::desc("File extensions that regard as header file.\n"
----------------
alexfh wrote:
> hokein wrote:
> > alexfh wrote:
> > > We don't need a command-line flag for this. The regular way to configure clang-tidy is .clang-tidy files. However, if needed, check options can be configured from the command line via the `-config=` option. 
> > From my understanding, it is a global option for header-file-extensions, right? If we remove this command-line flag, there is only local `header-file-extensions` option remained for particular checks.
> Both local and global options reside in `CheckOptions` and can be configured in the same way using either a .clang-tidy file or the -config= command-line option. The only difference between local and global options is that the name of the latter is not prefixed with the "CheckName.". Makes sense?
> Both local and global options reside in CheckOptions and can be configured in the same way using either a .clang-tidy file or the -config= command-line option. The only difference between local and global options is that the name of the latter is not prefixed with the "CheckName.". Makes sense?

Ah, thank you for that explanation. I learned something new today. ;-)


Repository:
  rL LLVM

http://reviews.llvm.org/D16113