kristina added a comment.

In https://reviews.llvm.org/D54344#1294942, @erik.pilkington wrote:

> LGTM too, with one small fix in test. Thanks for working on this!


Well, I figured since the assertion being tripped was (before investigation) 
the only reliable way to notice the bug it makes sense for it to stay in, main 
concern being that should anything regress and assertions are off, the code 
generation is essentially undefined. Though a good argument for taking it out 
would be the fact that currently it's the only test that verifies IR generated 
with the attribute (last time I checked), but I would also imagine most people 
running tests would have assertion enabled (or debug) builds.

Though I'm happy to revise the test to remove the assertion requirement, 
deferring to your judgement with regards to above.

Aside from that, are you happy for me to land this after the revision?


Repository:
  rC Clang

https://reviews.llvm.org/D54344



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to