[PATCH] D149757: Test data for symbol lookup. NFC

Fri Sep 15 22:14:20 PDT 2023

dblaikie added a comment.

In D149757#4635969 <https://reviews.llvm.org/D149757#4635969>, @sepavloff wrote:

> I don't think building test binaries in test time is possible for this particular case. The tests using the binary from here expect symbols at concrete locations. If something in code generation changes, and the locations become different, the tests would fail.
>
> In D149757#4549272 <https://reviews.llvm.org/D149757#4549272>, @ikudrin wrote:
>
>> This patch only adds files to be used in another patch. It does not change any code or add any new tests and has no value of its own. It should be merged with the main patch, or relevant parts of D149759 <https://reviews.llvm.org/D149759> should be moved here, whichever is preferable.
>
> You are right, these changes are a part of D149759 <https://reviews.llvm.org/D149759>. They are however independent of it and moving them to a separate patch could facilitate the review process.

Precommitting tests is encouraged by some (I'm not a huge fan, but I appreciate the benefits) - the purpose is to precommit a running test that demonstrates the existing (problematic/broken/buggy/suboptimal) behavior, then when the fix is committed, it includes only the changes to the test that demonstrate the improvement/fix - this makes it easier to see the change the test causes (rather than seeing the whole test and being unsure about what part of what the test is testing is new/different behavior)

Committing  the files without executing them as a test doesn't provide this value and shouldn't be done - then the files are committed without any verification of their behavior, obscuring their intent in this commit and in the subsequent bug fix.

Please either include a running test that verifies the buggy behavior in a precommit like this, or roll these files into the fix commit.

If @jhenderson would prefer them separate for review, but combined for commit, that's OK/I'll leave it up to him.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149757