[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