[PATCH] D43248: [Attr] Fix parameter indexing for attributes

[Eric Christopher via Phabricator via cfe-commits]

echristo added inline comments.


================
Comment at: test/Sema/attr-print.cpp:3
 
+// This file is also used as input for %S/../Frontend/ast-attr.cpp.
+
----------------
jdenny wrote:
> echristo wrote:
> > Relatedly I don't think we use files as input files to other directories and I don't think we should really. What are you trying to test here? This breaks the hermeticness of each particular test directory.
> Grep for "%S/.." and you'll see that a few other tests do something like this as well.
> 
> In test/Sema/attr-print.cpp, I am testing printing of attributes.  I chose to put that there because of the existing attr-print.c there.
> 
> In test/Frontend/ast-attr.cpp, I am testing serialization of attributes.  I chose to put that there because I see other -emit-ast tests there and because, if I put it in test/Sema instead, I get the complaint "Do not use the driver in Sema tests".
> 
> The same C++ code makes sense in both of these, but replicating it in two files would worsen maintainability.
> 
> I could try to  combine into one file in, say, tests/Misc if that works.
> 
> I have no strong opinions on where these live.  Just for my own education, is something real breaking right now because of their current locations?
> 
> Thanks.
Basically it's breaking an external build system (bazel) that has fairly distinct and specific dependency requirements and so layering and other dependencies are pretty entertaining.

Right now we avoid testing those particular tests and have TODOs of fixing them and the requirements to use a single directory and I was trying to avoid one more here.

All of that said I totally understand the desire to keep the maintenance burden minimized and an external build system shouldn't affect whether or not we do one particular thing or another - I was trying to get it written so that we could enable it without much undue burden. I'm uncertain if a comment of:

// If you change this file you should also change blah file.

or moving it to another directory where you can do both tests at the same time.


Repository:
  rC Clang

https://reviews.llvm.org/D43248