[PATCH] D154280: [LIT] Added an option to llvm-lit to emit the necessary test coverage data, divided per test case

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1087
+                env = dict(test.config.environment)  # Create a copy of the environment
+                env["LLVM_PROFILE_FILE"] = llvm_profile_file
+                commands[j] = f"export LLVM_PROFILE_FILE={llvm_profile_file} && {commands[j]}"
----------------
xgupta wrote:
> DavidSpickett wrote:
> > This appears to get set but never used. Or I am missing something later in the code, if that's the case, consider making that more obvious e.g. declaring it outside the for loop.
> Actually, this variable is being set for each RUN line in a test case so it is in the loop.
That explains it being in the loop, but where is `env` actually used?

It makes a copy of the current environment, and sets a new value in it:
```
env = dict(test.config.environment)  # Create a copy of the environment
env["LLVM_PROFILE_FILE"] = llvm_profile_file
```
However it doesn't use `env` again, instead it sets `llvm_profile_file` directly:
```
commands[j] = f"export LLVM_PROFILE_FILE={llvm_profile_file} && {commands[j]}"
coverage_index += 1
```
Which seems correct, so is `env` actually unused?

If it's used later outside the loop it would be whatever the last `env` set, which would likely create some strange results.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154280