erichkeane added a comment.

In D122983#3426661 <https://reviews.llvm.org/D122983#3426661>, @paulwalker-arm 
wrote:

> Thanks for this.  I can see about cleaning up the tests but I'm still not 
> sure what the advantage is.  The affected RUN lines are already 
> `-fsyntax-only` tests.  Is it about where the test files live or is there 
> something else I should be considering?  The benefit of the current tests is 
> that it's easy to spot holes as a single function tests both requirements.  
> My fear is that separating the tests based on Sema/CodeGen could mean we'll 
> miss something and never know.

There are two big problems with the way these are split up.  First, the 'list' 
of these functions that you expect to not be declared under a flag don't exist 
all in 1 place, and end up being difficult to follow/track down as they are 
spread out as much as they are.  Second, they are in tests that have a 
"REQUIRES" clause in it, which causes them to not be caught in many 
configurations.  We typically avoid doing -verify in CodeGen (unless the 
diagnostic is ACTUALLY in CodeGen) as a matter of business.  The diagnostic 
you're using to validate these is likely a poor choice (as it is a non-standard 
extension based warning) though, which is what makes this so difficult on folks 
trying to modernize Clang to follow standards.  IMO, if these had been C++ 
tests (which would have 'not found' failures), or validated the existence of 
these in some other mechanism, we likely would have never noticed other than 
via an audit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122983/new/

https://reviews.llvm.org/D122983

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

Reply via email to