[PATCH] D27502: [PM] Edit comments on PM Proxy and utility classes.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: llvm/include/llvm/IR/PassManager.h:882-883
 
-/// \brief A module analysis which acts as a proxy for a function analysis
-/// manager.
+/// \brief An analysis over a "larger" IR unit that provides access to an
+/// analysis manager over a "smaller" IR unit.
 ///
----------------
FWIW, I continue to prefer "outer" and "inner". There is an inherent requirement that the smaller IR units in question be *contained* by the larger units. I'd like words that help indicate that relationship which is a stronger statement than their size IMO.


================
Comment at: llvm/include/llvm/IR/PassManager.h:885-895
+/// For example, you might have an
+/// InnerAnalysisManagerProxy<FunctionAnalysisManager, Module>.  This is an
+/// analysis over Modules that provides access to a Function analysis manager.
+/// (The FunctionAnalysisManager is the "inner" manager being proxied.)
 ///
-/// Note that the proxy's result is a move-only object and represents ownership
-/// of the validity of the analyses in the \c FunctionAnalysisManager it
-/// provides.
+/// You should never use a "smaller" analysis manager from within (transitively)
+/// a "larger" pass manager unless you have received a proxy result object for
----------------
All of this is fantastic though. =]


================
Comment at: llvm/include/llvm/IR/PassManager.h:903
   public:
-    explicit Result(AnalysisManagerT &InnerAM) : InnerAM(&InnerAM) {}
-    Result(Result &&Arg) : InnerAM(std::move(Arg.InnerAM)) {
-      // We have to null out the analysis manager in the moved-from state
-      // because we are taking ownership of the responsibilty to clear the
-      // analysis state.
-      Arg.InnerAM = nullptr;
-    }
+    explicit Result(AnalysisManagerT &AM) : AM(&AM) {}
+    Result(Result &&RHS) : AM(std::move(RHS.AM)) { RHS.AM = nullptr; }
----------------
Why the less precise member name?

There are places where 'AM' is used and is *not* the same as the member, so it seems less ambiguous to give it some more specific name.


================
Comment at: llvm/include/llvm/IR/PassManager.h:904
+    explicit Result(AnalysisManagerT &AM) : AM(&AM) {}
+    Result(Result &&RHS) : AM(std::move(RHS.AM)) { RHS.AM = nullptr; }
     Result &operator=(Result &&RHS) {
----------------
I dislike using RHS for something that is not a binary operator. A lot of this delta isn't comment-only edits...


================
Comment at: llvm/include/llvm/IR/PassManager.h:994-995
+/// recursive return path of each layer of the pass manager.  A consequence of
+/// this is, the "larger" analyses may be stale.  We update the "larger"
+/// analyses only when we're done running passes over the "smaller" IR units.
 template <typename AnalysisManagerT, typename IRUnitT, typename... ExtraArgTs>
----------------
s/update/invalidate/


================
Comment at: llvm/include/llvm/IR/PassManager.h:1132
+    //
+    // We also preserve all analyses on Function units, because we did all the
+    // invalidation we needed to do above.
----------------
on Function units -> on Functions


================
Comment at: llvm/include/llvm/IR/PassManager.h:1239
 
-}
+} // namespace llvm
 
----------------
This isn't common for full-file-scoped namespaces in LLVM. See the last paragraph of http://llvm.org/docs/CodingStandards.html#namespace-indentation


https://reviews.llvm.org/D27502