Refactoring the dominators patch for Clang

Hi Guoping,

Thanks for the new patch! Looks good overall. Below are some minor comments.

Comments for the LLVM patch:1) In include/llvm/Analysis/Dominators.h, use typedef instead of spelling out GraphTraits<FT*> each time it’s used.

  1. In include/llvm/Support/CFG.h, use unsigned instead of size_t to be consistent with Function::size().

  2. The size function should probably be mentioned in the interface itself llvm/ADT/GraphTraits.h (not only in the template instantiations).

  3. Please, send the llvm patch to llvm-commits <> list for review.

Comments on the clang patch:

  1. I’d rather keep the old high level comments. For example, in Dominators.h we should specify what functionality the class/file provides, not how it’s implemented (Specifically, this is an implementation detail: “A wrapper to LLVM dominators implementation”). You can provide implementation details as well, but avoid putting those in headlines. It’s also better to use doxygen style comments:
  • /// changeImmediateDominator - This method is used to update the dominator
  • /// tree information when a node’s immediate dominator changes.
  • /// \brief This method is used to update the dominator
  • /// tree information when a node’s immediate dominator changes.
  1. iterator2 is not a very descriptive name :slight_smile:

  2. size_t → unsigned.

  3. There are a few unused APIs in DominatorTree (Ex: splitBlock()). Ideally, we’d want to keep as many of them as possible and just add test cases exercising them. However, you can completely remove some if they don’t seem very important; we can always add them in later on.


Hi, Anna

These comments are very helpful. Attached is the revised patch based on your comments.
Please let me know if you have additional comments.

dominators-clang.patch (17.2 KB)

dominators-llvm.patch (4.66 KB)

Also, I have submitted the llvm side patch to llvm-commits for review.

Hi Guoping,

I’ve added a couple of cleanups(violations of the 80 col rule, comments) and committed the patch in r145858!

As I mentioned earlier, I think it would also be beneficial to exercise some of the added API with the DebugChecker.