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