<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jan 24, 2014 at 5:00 AM, Artyom Skrobov <span dir="ltr"><<a href="mailto:Artyom.Skrobov@arm.com" target="_blank">Artyom.Skrobov@arm.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thank you for your review Sean<br>
<div class="im"><br>
> +// This needs to be shared between the instantiations of Find<><br>
> +typedef std::pair<std::string,std::string> KeyWithType;<br>
> +static SmallSet<KeyWithType,10> reportedAsUnrecognized;<br>
><br>
</div><div class="im">> This is introducing a global into the libraries, which is<br>
> verboten. Please revert and find a design that doesn't require<br>
> the global.<br>
> This should have been caught in pre-commit review.<br>
<br>
</div>This global is not very global (namely, it's static); besides that,<br>
<a href="http://llvm.org/docs/CodingStandards.html" target="_blank">http://llvm.org/docs/CodingStandards.html</a> doesn't include any prohibition of<br>
globals.<br>
<br>
Should I turn it into a static member of SubtargetFeatures class?<br>
This wouldn't reduce neither the lifetime nor the visibility of this<br>
variable, but would probably make it less eye-catching.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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).</div>
<div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
<br>
> +  /// Find KV in array using binary search.<br>
> +  /// T should be either SubtargetFeatureKV or SubtargetInfoKV<br>
> +  template<typename T><br>
> +  static const T *Find(StringRef Key, const T *Array, size_t Length,<br>
> +                       const char* KeyType);<br>
><br>
> This should be `find` according to the naming convention. Also,<br>
> You should use ArrayRef instead of a raw pointer + length.<br>
> Maybe a more descriptive name than `find` would be useful as<br>
> well.<br>
<br>
</div>This function has existed, has been called 'Find', and has been taking raw<br>
pointer + length, since SubtargetFeature.cpp was originally created in 2005<br>
(r23191).</blockquote><div> </div></div><br></div></div>