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

[Sean Silva]

On Fri, Jan 24, 2014 at 5: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.
>

Our documentation is not very good surrounding some of these larger
architectural issues. However, I can tell you with certainty that this
patch is unacceptable in it's current state. Revert it.

As David said, it violates a fundamental invariant of the LLVM codebase,
which is that library users in the same process can independently use the
LLVM library. (There are some unfortunate historical deviations from this
that are being slowly mended, but going new code doesn't regress on this:
it's not up for debate).

-- Sean Silva


>
>
> > +  /// 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).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140124/9237a276/attachment.html>