<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jan 24, 2014 at 2: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>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)<br>
<br>LLVM has gone to substantial lengths to support this scenario (see LLVMContext) - it's not an invariant to be easily discarded.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class="im">> +  /// 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><br></div><div>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...) </div>
</div></div></div>