[llvm] r199886 - Prevent repetitive warnings for unrecognized processors and features
Artyom Skrobov
Artyom.Skrobov at arm.com
Fri Jan 24 02:00:23 PST 2014
Thank you for your review Sean
> +// This needs to be shared between the instantiations of Find<>
> +typedef std::pair<std::string,std::string> KeyWithType;
> +static SmallSet<KeyWithType,10> reportedAsUnrecognized;
>
> This is introducing a global into the libraries, which is
> verboten. Please revert and find a design that doesn't require
> the global.
> This should have been caught in pre-commit review.
This global is not very global (namely, it's static); besides that,
http://llvm.org/docs/CodingStandards.html doesn't include any prohibition of
globals.
Should I turn it into a static member of SubtargetFeatures class?
This wouldn't reduce neither the lifetime nor the visibility of this
variable, but would probably make it less eye-catching.
> + /// Find KV in array using binary search.
> + /// T should be either SubtargetFeatureKV or SubtargetInfoKV
> + template<typename T>
> + static const T *Find(StringRef Key, const T *Array, size_t Length,
> + const char* KeyType);
>
> This should be `find` according to the naming convention. Also,
> You should use ArrayRef instead of a raw pointer + length.
> Maybe a more descriptive name than `find` would be useful as
> well.
This function has existed, has been called 'Find', and has been taking raw
pointer + length, since SubtargetFeature.cpp was originally created in 2005
(r23191).
More information about the llvm-commits
mailing list