[PATCH] D34660: [CFLAA] Move a common function to the utils header to reduce duplication

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!



================
Comment at: include/llvm/Analysis/CFLAliasAnalysisUtils.h:23
 
 template <typename AAResult> struct FunctionHandle final : public CallbackVH {
   FunctionHandle(Function *Fn, AAResult *Result)
----------------
Now that I look at it, this should probably live in the `llvm::cflaa` namespace, rather than the global namespace (unless you're planning on making this more general?) As an added bonus, that may let us get rid of the `using namespace llvm` above?

I'm indifferent about whether that's included in this patch, or if it's done in a follow-up.


================
Comment at: include/llvm/Analysis/CFLAliasAnalysisUtils.h:46
+namespace cflaa {
+inline const Function *parentFunctionOfValue(const Value *Val) {
+  if (auto *Inst = dyn_cast<Instruction>(Val)) {
----------------
> I'm not convinced about using inline in the header to force the function to live in a COMDAT, so, suggestions welcome

Honestly, given how small this function will end up being and that it should only ever be used in 2 TUs, I'd be content if we just made it `static inline`.

I'm happy with either approach ¯\_(ツ)_/¯.


================
Comment at: include/llvm/Analysis/CFLAliasAnalysisUtils.h:56
+}
+}
+}
----------------
If we expand these namespaces to include `FunctionHandle`, please add `// namespace cflaa` and `// namespace llvm` to their respective close braces.


https://reviews.llvm.org/D34660