[llvm] r199886 - Prevent repetitive warnings for unrecognized processors and features

[David Blaikie]

On Fri, Jan 24, 2014 at 2:00 AM, Artyom Skrobov <Artyom.Skrobov at arm.com>wrote:

> 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.
>

It's not a question of how eye-catching it is. It's a practical issue about
how this impacts the reusability of LLVM - eg: two independent users of the
LLVM library in the same process would have substantial problems with this
change, would they not? (racing, if nothing else)

LLVM has gone to substantial lengths to support this scenario (see
LLVMContext) - it's not an invariant to be easily discarded.

> +  /// 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).


And LLVM's coding conventions have changed/improved since then. It's nice,
but not a must-have, to bring things "up to code" as it were, where
convenient (and I assume you were just moving this function from one place
to another? From the patch it looks like you're adding it...)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140124/ddca6721/attachment.html>