[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.
pierre gousseau via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 28 09:04:15 PST 2019
pgousseau marked 2 inline comments as done.
pgousseau added inline comments.
Comment at: include/clang/Basic/Sanitizers.h:29
> pgousseau wrote:
> > riccibruno wrote:
> > > What ? You are forward-declaring `hash_code` here and using it as a value a few lines later. Just include `llvm/ADT/Hashing.h`.
> > Thanks for reviewing!
> > I am happy to include `Hashing.h` but I thought it was generally frown upon to add includes if avoidable?
> > `hash_code` is only used in the return type of the `hash_value()` declaration here no?
> > I saw StringRef.h or APInt.h also forward declare `hash_code`.
> > Thanks again for reviewing and sorry for the many iterations.
> Never mind, I overlooked that it is only the declaration of `llvm::hash_code hash_value(const clang::SanitizerMask &Arg);` and not the definition. Sorry about that.
> I believe that is it correct to have the overload of `hash_value` in the namespace `clang`, so that it can be found by ADL since `SanitizerMask` is in `clang`. However you can probably move the declaration of `llvm::hash_code hash_value(const clang::SanitizerMask &Arg);` just after `SanitizerMask` so that you don't have to forward-declare `SanitizerMask` (but keep the forward-declaration of `hash_code` in `llvm`).
Sounds good, done!
CHANGES SINCE LAST ACTION
More information about the cfe-commits