Cleanup

Author: john-h-kastner

clang/include/clang/3C/DeclRewriter.h

@@ -68,7 +68,7 @@ class DeclRewriter {
                         std::vector<Decl *> SameLineDecls,
                         bool ContainsInlineStruct);
   void rewriteSingleDecl(DeclReplacement *N, RSet &ToRewrite);
-  void doDeclRewrite(SourceRange SR, DeclReplacement *N);
+  void doDeclRewrite(SourceRange &SR, DeclReplacement *N);
   void rewriteFunctionDecl(FunctionDeclReplacement *N);
   void rewriteTypedefDecl(TypedefDeclReplacement *TDT, RSet &ToRewrite);
   void emitSupplementaryDeclarations(const std::vector<std::string> &SDecls,

clang/lib/3C/AVarBoundsInfo.cpp

@@ -359,17 +359,6 @@ void AvarBoundsInference::getRelevantBounds(BoundsKey BK,
   } else if (ABounds *PrevBounds = BI->getBounds(BK)) {
     ResBounds[PrevBounds->getKind()].insert(PrevBounds->getBKey());
   }
-
-
-  //for (auto &E : ResBounds) {
-  //  auto Iter = E.second.begin();
-  //  while (Iter != E.second.end()) {
-  //    if (BI->hasPointerArithmetic(*Iter))
-  //      Iter = E.second.erase(Iter);
-  //    else
-  //      ++Iter;
-  //  }
-  //}
 }
 
 bool AvarBoundsInference::areDeclaredBounds(
@@ -1018,6 +1007,8 @@ bool AVarBoundsInfo::hasPointerArithmetic(BoundsKey BK) {
   return ArrPointersWithArithmetic.find(BK) != ArrPointersWithArithmetic.end();
 }
 
+// A pointer needs range bounds if it is computed by pointer arithmetic and
+// would otherwise need bounds.
 bool AVarBoundsInfo::needsRangeBound(BoundsKey BK) {
   return hasPointerArithmetic(BK) && getBounds(BK) != nullptr;
 }

clang/lib/3C/DeclRewriter.cpp

@@ -36,15 +36,14 @@ void DeclRewriter::buildItypeDecl(PVConstraint *Defn, DeclaratorDecl *Decl,
                                   std::string &Type, std::string &IType,
                                   ProgramInfo &Info, ArrayBoundsRewriter &ABR,
                                   std::vector<std::string> &SDecls) {
-  // TODO: Is this sufficient to get the bounds key? More complex logic is used
-  //       elsewhere.
   bool NeedsRangeBound =
     Defn->hasBoundsKey() &&
     Info.getABoundsInfo().needsRangeBound(Defn->getBoundsKey());
 
   std::string DeclName = Decl ? Decl->getNameAsString() : "";
   if (NeedsRangeBound) {
     // TODO: Think about this assert. When is Decl null? When is the name empty?
+    //       One case that comes to mind if unnamed parameter declaration.
     assert(!DeclName.empty());
     DeclName = "__3c_tmp_" + DeclName;
   }
@@ -98,7 +97,7 @@ void DeclRewriter::buildItypeDecl(PVConstraint *Defn, DeclaratorDecl *Decl,
       Type = qtyToStr(Decl->getType(), DeclName);
     else {
       // FIXME: This assert will probably fail. Need to handle field and global
-      //        decls and maybe returns. Also param decls without names.
+      //        decls, function returns and param decls without names.
       assert(!NeedsRangeBound);
       Type = Defn->getOriginalTypeWithName();
     }
@@ -110,31 +109,8 @@ void DeclRewriter::buildItypeDecl(PVConstraint *Defn, DeclaratorDecl *Decl,
                                         UnmaskTypedef = IsUncheckedTypedef));
   IType += ")" + ABR.getBoundsString(Defn, Decl, true, false);
 
-  // FIXME: The parameter has an itype because it's used unsafely somewhere. Now
-  //        we copy it into a local variable with a fully checked type. The
-  //        unsafe use that made the parameter an itype will now be an error.
-  //        Solutions:
-  //        - Leave it how it is. There's a compiler error that needs to be
-  //          fixed, but this is already expected. This error is not an bounds
-  //          error however, so we still want to avoid it.
-  //        - Prohibit pointer arithmetic on itypes. Loses bounds on parameter.
-  //        - Make the copy unchecked. Looses bounds on the local. Note that the
-  //          itype parameter was already treated as unchecked inside the
-  //          function, so this should be a correct rewriting. Maybe add bounds
-  //          in a comment.
-  //        - Real solution: Use the internal type of the typedef (after fixing)
-  //          #704. [ leaning towards this ]
+  // We don't insert a bounds string because this is an unchecked pointer.
   if (NeedsRangeBound)
-    // FIXME: This isn't an itype base; although, it's an unchecked alias for a
-    //        itype which is conceptually kind of the same thing. ForItypeBase
-    //        needs a better name, but it can't just be `EmitUnchecked` because
-    //        it also controls function pointer parameter rewriting with extra
-    //        parens.
-    // We don't insert a bounds string because this is an unchecked pointer.
-    // TODO: If #704 is fixed, then we call mkString on the internal constraint
-    //       variable without ForItypeBase=true. Then, if the top level of the
-    //       internal constraint is checked (inner pointer levels don't matter),
-    //       we can add the bounds string.
     SDecls.push_back(Defn->mkString(Info.getConstraints(),
                                     MKSTRING_OPTS(ForItypeBase = true)) +
                      " = " + DeclName + ";");
@@ -150,9 +126,6 @@ void DeclRewriter::rewriteDecls(ASTContext &Context, ProgramInfo &Info,
   // as well as their rewriten types in a map.
   RSet RewriteThese;
 
-  //
-  std::map<Decl *, std::string> DuplicatedDecls;
-
   FunctionDeclBuilder *TRV = nullptr;
 #ifdef FIVE_C
   auto TRV5C = FunctionDeclBuilder5C(&Context, Info, RewriteThese, ABRewriter);
@@ -261,8 +234,6 @@ void DeclRewriter::rewriteDecls(ASTContext &Context, ProgramInfo &Info,
           NewTy += Type + IType;
         } else {
 
-          // TODO: Same question about getBoundsKey as above. Also, copy-pasted
-          //       code.
           bool NeedsRangeBounds =
             PV->hasBoundsKey() &&
             Info.getABoundsInfo().needsRangeBound(PV->getBoundsKey());
@@ -529,8 +500,7 @@ void DeclRewriter::rewriteMultiDecl(DeclReplacement *N, RSet &ToRewrite,
 // Common rewriting logic used to replace a single decl either on its own or as
 // part of a multi decl. The primary responsibility of this method (aside from
 // invoking the rewriter) is to add any required initializer expression.
-void DeclRewriter::doDeclRewrite(SourceRange SR, DeclReplacement *N) {
-  // Save original end of source range in case it changes
+void DeclRewriter::doDeclRewrite(SourceRange &SR, DeclReplacement *N) {
   std::string Replacement = N->getReplacement();
   if (isa<TypedefDecl>(N->getDecl()))
     Replacement = "typedef " + Replacement;
@@ -561,12 +531,6 @@ void DeclRewriter::doDeclRewrite(SourceRange SR, DeclReplacement *N) {
   // Lexer::findNextToken can fail (why?). Rather than adding extra error
   // handling here, pass an invalid location to emitSupplementryDeclaration so
   // that it can emit an error.
-  // TODO: Is worth emitting a separate error here? We would be able to log the
-  //       the location passed to the failed findNextToken call. If adding an
-  //       error here, consider that it should only be emitted if rewriting
-  //       would actually need to happen. For test macro_rewrite_error.c, the
-  //       findNextToken call fails, but the invalid source location is never
-  //       used by emitSupplementaryDeclarations, so there is no error.
   Optional<Token> NextToken = Lexer::findNextToken(
     N->getSourceRange(A.getSourceManager()).getEnd(), A.getSourceManager(),
     A.getLangOpts());
@@ -921,8 +885,6 @@ void FunctionDeclBuilder::buildCheckedDecl(
     std::string &IType, std::string UseName, bool &RewriteParm,
     bool &RewriteRet, std::vector<std::string> &SDecls) {
 
-  // TODO: What does it mean for a defn to not have a bounds key? Presumably it
-  //       can't have bounds, and therefore can't need range bounds.
   bool NeedsRangeBound =
     Defn->hasBoundsKey() &&
     Info.getABoundsInfo().needsRangeBound(Defn->getBoundsKey());

clang/lib/3C/RewriteUtils.cpp

@@ -716,4 +716,3 @@ void RewriteConsumer::HandleTranslationUnit(ASTContext &Context) {
   Info.exitCompilationUnit();
   return;
 }
-