[NFC] Cleanup: BasicBlock::getInstList() is now private.

[vporpo]

Summary

We now have an adequate set of API functions that we shouldn't need access
to the underlying instruction list.

[aeubanks]

llvm/include/llvm/IR/BasicBlock.h

92

instruction

386–387

this should also be private, although I'd hope people don't abuse this

looks like it's used as part of ilist_node_with_parent, maybe friend class that?

[vporpo]

llvm/include/llvm/IR/BasicBlock.h

386–387

Yeah good point. I made this private too.

[vporpo]

Addressed comments.

[aeubanks]

presubmit bot is saying there's a missed usage in BPFAbstractMemberAccess.cpp:1147

[aeubanks]

for changes like this, keep an eye on the presubmit bots ("x64 debian failed")

[vporpo]

Oops good catch, thanks! I should have tested with all targets enabled.

[vporpo]

Rebased.

[vporpo]

Rebased.

[vporpo]

The build bots were complaining about a formatting issue in nearby code, so I clang-formatted that too.

[aeubanks]

yay finally

[steven_wu]

This breaks llvm/example:

/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/examples/ModuleMaker/ModuleMaker.cpp:58:7: error: 'getInstList' is a private member of 'llvm::BasicBlock'

See: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/33265/console

[vporpo]

Thanks for reporting this. I will push a fix soon.

[aeubanks]

grepping for getInstList shows that BrainF.cpp also uses it
should also update ProgrammersManual.rst

[vporpo]

Both examples should now be fixed. Updated the doc too. Thanks for your help @aeubanks .

llvm/include/llvm/IR/BasicBlock.h

public:
   LLVMContext &getContext() const;
 
   /// Instruction iterators...
   using iterator = InstListType::iterator;
   using const_iterator = InstListType::const_iterator;
   using reverse_iterator = InstListType::reverse_iterator;
   using const_reverse_iterator = InstListType::const_reverse_iterator;
 
+  // These functions and classes need access to the instruction list.
+  friend void Instruction::removeFromParent();
+  friend iplist<Instruction>::iterator Instruction::eraseFromParent();
+  friend BasicBlock::iterator Instruction::insertAt(BasicBlock *BB,
+                                                    BasicBlock::iterator It);
+  friend class llvm::SymbolTableListTraits<llvm::Instruction>;
+  friend class llvm::ilist_node_with_parent<llvm::Instruction, llvm::BasicBlock>;
+
   /// Creates a new BasicBlock.
   ///
   /// If the Parent parameter is specified, the basic block is automatically
   /// inserted at either the end of the function (if InsertBefore is 0), or
   /// before the specified basic block.
   static BasicBlock *Create(LLVMContext &Context, const Twine &Name = "",
                             Function *Parent = nullptr,
                             BasicBlock *InsertBefore = nullptr) {
public:
   /// Returns a range that iterates over the phis in the basic block.
   ///
   /// Note that this cannot be used with basic blocks that have no terminator.
   iterator_range<const_phi_iterator> phis() const {
     return const_cast<BasicBlock *>(this)->phis();
   }
   iterator_range<phi_iterator> phis();
 
+private:
   /// Return the underlying instruction list container.
-  ///
-  /// Currently you need to access the underlying instruction list container
-  /// directly if you want to modify it.
+  /// This is deliberately private because we have implemented an adequate set
+  /// of functions to modify the list, including BasicBlock::splice(),
+  /// BasicBlock::erase(), Instruction::insertAt() etc.
   const InstListType &getInstList() const { return InstList; }
-        InstListType &getInstList()       { return InstList; }
+  InstListType &getInstList() { return InstList; }
 
   /// Returns a pointer to a member of the instruction list.
-  static InstListType BasicBlock::*getSublistAccess(Instruction*) {
+  /// This is private on purpose, just like `getInstList()`.
+  static InstListType BasicBlock::*getSublistAccess(Instruction *) {
     return &BasicBlock::InstList;
   }
 
+public:
   /// Returns a pointer to the symbol table if one exists.
   ValueSymbolTable *getValueSymbolTable();
 
   /// Methods for support type inquiry through isa, cast, and dyn_cast.
   static bool classof(const Value *V) {
     return V->getValueID() == Value::BasicBlockVal;
   }
 

llvm/lib/IR/BasicBlock.cpp

#ifdef EXPENSIVE_CHECKS
   for (auto It = FromBeginIt; It != FromEndIt; ++It)
     assert(It != FromBBEnd && "FromBeginIt not before FromEndIt!");
 #endif // EXPENSIVE_CHECKS
   getInstList().splice(ToIt, FromBB->getInstList(), FromBeginIt, FromEndIt);
 }
 
 BasicBlock::iterator BasicBlock::erase(BasicBlock::iterator FromIt,
                                        BasicBlock::iterator ToIt) {
-  return getInstList().erase(FromIt, ToIt);
+  return InstList.erase(FromIt, ToIt);
 }
 
 void BasicBlock::replacePhiUsesWith(BasicBlock *Old, BasicBlock *New) {
   // N.B. This might not be a complete BasicBlock, so don't assume
   // that it ends with a non-phi instruction.
   for (Instruction &I : *this) {
     PHINode *PN = dyn_cast<PHINode>(&I);
     if (!PN)