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

[Shivam Gupta via Phabricator via llvm-commits]

xgupta marked an inline comment as done.
xgupta 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]}"
----------------
DavidSpickett wrote:
> 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.
Thanks, @DavidSpickett, yes you are correct, we are not using env variable in this code. 
Actually that we were using it before when setting LLVM_PROFILE_FILE for each file (.script), but now it should be for each RUN line so use a different approach. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154280