[tools] fix Wdangling-assignment-gsl
#8407
Conversation
makslevental
force-pushed
the
makslevental/bump-llvm
branch
2 times, most recently
from
3acf2eb to
f6b9046
Compare
makslevental
requested review from
darthscsi,
fabianschuiki,
maerhart and
seldridge
as code owners
makslevental
force-pushed
the
makslevental/bump-llvm
branch
from
f6b9046 to
5678a83
Compare
tools/arcilator/arcilator.cpp
Outdated
| engineOptions.transformer = mlir::makeOptimizingTransformer( | ||
| /*optLevel=*/3, /*sizeLevel=*/0, | ||
| /*targetMachine=*/nullptr); | ||
| static std::function<llvm::Error(llvm::Module *)> transformer = |
There was a problem hiding this comment.
otherwise
warning: object backing the pointer engineOptions.transformer will be destroyed
at the end of the full-expression [-Wdangling-assignment-gsl]
because engineOptions.transformer is a function_ref.
There was a problem hiding this comment.
Is using a global variable the approved way of doing this upstream? I suspect a auto transformer = ...; engineOptions.transformer = transformer; might suffice 🤔
There was a problem hiding this comment.
it's locally allocated/scope - it'll die at the end of the body while the executionEngine is returned
There was a problem hiding this comment.
To me it looks like an auto transformer = ... would have the same lifetime as the execution engine. Where is the engine returned?
There was a problem hiding this comment.
oh um whoops - I skimmed the file too quickly (the return is success). Lemme change.
|
@makslevental I had started an LLVM update here: #8404. Should I get that one in before your SMT-focused changes? |
ah sorry didn't notice that one. yea this one needs to precede the SMT integration. looks like the only diff between mine and yours is this thing with |
|
Ok, I merged mine. Sorry for the delay and apparent merge conflicts. Hopefully simple to rebase and just deal with the function_ref thing. |
makslevental
force-pushed
the
makslevental/bump-llvm
branch
from
5678a83 to
7177f88
Compare
makslevental
changed the title
Wdangling-assignment-gsl
|
Okay I rebased and renamed this PR to just be about |
tools/arcilator/arcilator.cpp
Outdated
| engineOptions.transformer = mlir::makeOptimizingTransformer( | ||
| /*optLevel=*/3, /*sizeLevel=*/0, | ||
| /*targetMachine=*/nullptr); | ||
| static std::function<llvm::Error(llvm::Module *)> transformer = |
There was a problem hiding this comment.
To me it looks like an auto transformer = ... would have the same lifetime as the execution engine. Where is the engine returned?
tools/circt-lec/circt-lec.cpp
Outdated
| mlir::ExecutionEngineOptions engineOptions; | ||
| engineOptions.transformer = mlir::makeOptimizingTransformer( | ||
| /*optLevel*/ 3, /*sizeLevel=*/0, /*targetMachine=*/nullptr); | ||
| static std::function<llvm::Error(llvm::Module *)> transformer = |
There was a problem hiding this comment.
wouldn't a std::function<llvm::Error(llvm::Module *)> transformer; declaration above where the engine variable is declared (i.e. outside this local scope) be enough (instead of using static here)?
makslevental
force-pushed
the
makslevental/bump-llvm
branch
from
7177f88 to
5983e60
Compare
tools/circt-bmc/circt-bmc.cpp
Outdated
| mlir::ExecutionEngineOptions engineOptions; | ||
| engineOptions.transformer = mlir::makeOptimizingTransformer( | ||
| /*optLevel*/ 3, /*sizeLevel=*/0, /*targetMachine=*/nullptr); | ||
| std::function<llvm::Error(llvm::Module *)> transformer = |
There was a problem hiding this comment.
Don't you have to declare the variable outside this nested scope, because otherwise it'd be deallocated at the engine->invokePacked call, right? (also in circt-lec.cpp)
makslevental
force-pushed
the
makslevental/bump-llvm
branch
from
5983e60 to
4e939ac
Compare
4 participants
This PR fixes
Wdangling-assignment-gslincirct-bmc,circt-lec, andarcilator.