erik.pilkington added a comment. In https://reviews.llvm.org/D54344#1294960, @kristina wrote:
> 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. clang/test/CodeGenCXX/{always,no}_destroy.cpp also test the generated IR. I would prefer always running it, its an interesting code path that might break in other ways, so even if we can't test as thoroughly without assertions, its still worth it to run. > 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? Yep, land away! Thanks again for fixing this. 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
