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

Alexander Kornienko via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 13 03:16:01 PST 2016


alexfh added inline comments.

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


Repository:
  rL LLVM

http://reviews.llvm.org/D16113





More information about the cfe-commits mailing list