[PATCH] D52999: [FileCheck] Annotate input dump (1/7)

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In https://reviews.llvm.org/D52999#1301285, @probinson wrote:

> It really seems like DiagList and AnnotationList ought to be vectors, not lists.  They are append-only, and AnnotationList gets converted to a vector anyway to sort it.  The code doesn't depend on the element-pointer stability guarantee of a list, except in one place noted below which can be fixed.
>  It's quite possible a vector would perform less well, in the face of many diags/annotations, but as the diags/annotations are the unusual case, performance is not really a big consideration.


I was thinking of many diags (-v with many checks) because that's where the annotations have proven most helpful to me as a visualization.  I was thinking the conversion to vector would be a small penalty in comparison to the potentially many copies during reiszing the vector. I was more concerned about the time impact than the space impact, which I guessed would be small on a modern system.

Having said all that, I don't have a strong opinion here, and we can change it later if someone does some performance measurements, so I'd be happy to use vector now with no further discussion if my above arguments aren't convincing. Just say the word.

In https://reviews.llvm.org/D52999#1301364, @probinson wrote:

> Regarding the bit about environment variables, probably the right thing to do is add a lit.local.cfg to the FileCheck test directory, that zaps the environment variables.  Then most individual FileCheck tests can assume a default environment, and tests for specific non-defaults can set the environment variables directly in the test file.
>  It would mean you can't run the FileCheck tests *from lit* with an environment variable set.  Unless we invented another env var that lit.local.cfg would look for... but that's for another patch, at this point.


That's fine as a first step to addressing that issue. However we do it, I think it should move to a separate patch as the issue exists independently of this patch series. Agreed?

By the way, this issue also exists in the lit test suite.



================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:232
+  for (auto DiagItr = DiagList.begin(), DiagEnd = DiagList.end();
+       DiagItr != DiagEnd; ++DiagItr) {
+    AnnotationList.emplace_back();
----------------
probinson wrote:
> range-for here?
D53893 (later patch in this series) makes use of the iterators.  Let me know if you think there's a better way. 


https://reviews.llvm.org/D52999