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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
