*Almost* all uses are replaced. Left FIXMEs for the two sites that
require refactoring outside of Inliner, to scope this patch.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks good!
llvm/lib/Transforms/IPO/Inliner.cpp | ||
---|---|---|
949 | Use 'auto *' here, now that CS is a pointer type | |
1157 | Side note (not related to this patch (saw some other instances while reviewing this too, though) - but might be handy for reference otherwise) - generally for a locally used lambda like this (especially a lambda only used within its expression) I'd encourage using [&] rather than an explicit capture list - like any other nested scope, it should have access to all the enclosing variables implicitly. |
fix Harbormaster lint failure
llvm/lib/Transforms/IPO/Inliner.cpp | ||
---|---|---|
1157 | Ack - subsequent change. There are also naming inconsistencies (for (int i, instead of int I, etc)) |
llvm/lib/Transforms/IPO/Inliner.cpp | ||
---|---|---|
520 | Yeah - this looks like Harbormaster just couldn't tell you didn't introduce this problem (the 'message' name not following LLVM naming conventions) - so I'd generally advise ignoring such a suggestion, or fixing it in an independent commit - no big deal/wouldn't bother fussing about it if you'd rather just commit it together this time. |
I'd say "Same as other llvm::Instructions" - and I /assume/ the answer there is that they're still predominantly handled as pointers, rather than references?
Seems for /literally/ Instruction*/Instruction& (as opposed to all possible types of Instructions - harder to search/gather data for all of them) it's about 7:1 in favor of *, but that's hard to evaluate/compare - some of those may need to be pointers because null is an option.
Evidently references to Instructions aren't totally unheard of, so the preference "use references when something can't be null" could be observed here. A pretty clear endorsement of this is that "instructions(F)" gives you an iterator over Instruction&, not over Instruction*.
But yeah - given the current codebase I think it's dealer's choice & not something I'd push strongly either way in review personally. (but I don't do lots of Instruction handling, so maybe some other folks have stronger ideas about which direction this sort of thing should be going in)
This seems like a reasonable guideline, though - i.e. using references in function signatures where the contract is the parameter must be non-null. Looking at the refactored sites, there are a few spots where the caller is assumed to pass a valid CallSite. While that wasn't checked, now may be a good time to have that signal.
Example: setInlineRemark
But yeah - given the current codebase I think it's dealer's choice & not something I'd push strongly either way in review personally. (but I don't do lots of Instruction handling, so maybe some other folks have stronger ideas about which direction this sort of thing should be going in)
The old CallSite based interface is a mixed bag -- it is passed by value, but behaves like a reference sometimes but also supports 'null/boolean' check which is pointer like.
looks good
Yeah - this looks like Harbormaster just couldn't tell you didn't introduce this problem (the 'message' name not following LLVM naming conventions) - so I'd generally advise ignoring such a suggestion, or fixing it in an independent commit - no big deal/wouldn't bother fussing about it if you'd rather just commit it together this time.