[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
llvm-commits at lists.llvm.org
Tue Jul 25 22:17:35 PDT 2023
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
More information about the llvm-commits
mailing list