[AST] Replace asserts with if() in SourceLocation accessors

[steveire]

Summary

Nowhere else in the AST classes assert on these kinds of accessors.

This way, we can call the accessors and check the validity of the result
instead of externally duplicating the conditions. This generality will
make it possible to introspect instances for source locations:

http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/iiaWhw

[aaron.ballman]

The downside to this change is that callers now have to check the return values for validity where they didn't previously because the assertion covered it. However, I think that's probably reasonable in these cases since they're mostly returning source locations or ranges.

include/clang/AST/DeclarationName.h

733

Formatting is incorrect here and elsewhere -- you should run the patch through clang-format.

735

Did you investigate the callers of this function to see which ones need a null pointer check inserted into them? Otherwise, this change turns an assertion into harder to track down UB. (I'm less worried about the other changes because those will fail more gracefully.)

lib/AST/NestedNameSpecifier.cpp

465–467

Remove spurious parens.

[steveire]

Fix formatting

[steveire]

include/clang/AST/DeclarationName.h

735

All callers in clang are fine. Here's the output of git grep -h TypeSourceInfo:

/// getNamedTypeInfo - Returns the source type info associated to
TypeSourceInfo *getNamedTypeInfo() const {
  if (TypeSourceInfo *TSInfo = NameInfo.getNamedTypeInfo())
  if (auto ToTInfoOrErr = import(From.getNamedTypeInfo()))
    if (auto TypeNameInfo = Dtor->getNameInfo().getNamedTypeInfo()) {
  if (TypeSourceInfo *TSInfo = NameInfo.getNamedTypeInfo())
  if (TypeSourceInfo *OldTInfo = NameInfo.getNamedTypeInfo()) {
  if (TypeSourceInfo *TSInfo = Name.getNamedTypeInfo())

clang-tools-extra has no uses of it. Is there anywhere else to check?

[aaron.ballman]

LGTM aside from a formatting nit.

include/clang/AST/DeclarationName.h

735

I double-checked the call to import() and it gracefully handles null pointers as well, so I think this change is safe (I cannot find any further instances that you didn't find). Thank you for looking into it!

lib/AST/NestedNameSpecifier.cpp

465–467

Formatting is still off here for the second line of the if statement (one too many spaces on the indentation).

[steveire]

lib/AST/NestedNameSpecifier.cpp

465–467

Fixed, thanks. I ran c-f before fixing the parens.

include/clang/AST/DeclarationName.h

public:
 
   const DeclarationNameLoc &getInfo() const { return LocInfo; }
   DeclarationNameLoc &getInfo() { return LocInfo; }
   void setInfo(const DeclarationNameLoc &Info) { LocInfo = Info; }
 
   /// getNamedTypeInfo - Returns the source type info associated to
   /// the name. Assumes it is a constructor, destructor or conversion.
   TypeSourceInfo *getNamedTypeInfo() const {
-    assert(Name.getNameKind() == DeclarationName::CXXConstructorName ||
-           Name.getNameKind() == DeclarationName::CXXDestructorName ||
-           Name.getNameKind() == DeclarationName::CXXConversionFunctionName);
+    if (Name.getNameKind() != DeclarationName::CXXConstructorName &&
+        Name.getNameKind() != DeclarationName::CXXDestructorName &&
+        Name.getNameKind() != DeclarationName::CXXConversionFunctionName)
+      return nullptr;
     return LocInfo.NamedType.TInfo;
   }
 
   /// setNamedTypeInfo - Sets the source type info associated to
   /// the name. Assumes it is a constructor, destructor or conversion.
   void setNamedTypeInfo(TypeSourceInfo *TInfo) {
     assert(Name.getNameKind() == DeclarationName::CXXConstructorName ||
            Name.getNameKind() == DeclarationName::CXXDestructorName ||
            Name.getNameKind() == DeclarationName::CXXConversionFunctionName);
     LocInfo.NamedType.TInfo = TInfo;
   }
 
   /// getCXXOperatorNameRange - Gets the range of the operator name
   /// (without the operator keyword). Assumes it is a (non-literal) operator.
   SourceRange getCXXOperatorNameRange() const {
-    assert(Name.getNameKind() == DeclarationName::CXXOperatorName);
+    if (Name.getNameKind() != DeclarationName::CXXOperatorName)
+      return SourceRange();
     return SourceRange(
      SourceLocation::getFromRawEncoding(LocInfo.CXXOperatorName.BeginOpNameLoc),
      SourceLocation::getFromRawEncoding(LocInfo.CXXOperatorName.EndOpNameLoc)
                        );
   }
 
   /// setCXXOperatorNameRange - Sets the range of the operator name
   /// (without the operator keyword). Assumes it is a C++ operator.
   void setCXXOperatorNameRange(SourceRange R) {
     assert(Name.getNameKind() == DeclarationName::CXXOperatorName);
     LocInfo.CXXOperatorName.BeginOpNameLoc = R.getBegin().getRawEncoding();
     LocInfo.CXXOperatorName.EndOpNameLoc = R.getEnd().getRawEncoding();
   }
 
   /// getCXXLiteralOperatorNameLoc - Returns the location of the literal
   /// operator name (not the operator keyword).
   /// Assumes it is a literal operator.
   SourceLocation getCXXLiteralOperatorNameLoc() const {
-    assert(Name.getNameKind() == DeclarationName::CXXLiteralOperatorName);
+    if (Name.getNameKind() != DeclarationName::CXXLiteralOperatorName)
+      return SourceLocation();
     return SourceLocation::
       getFromRawEncoding(LocInfo.CXXLiteralOperatorName.OpNameLoc);
   }
 
   /// setCXXLiteralOperatorNameLoc - Sets the location of the literal
   /// operator name (not the operator keyword).
   /// Assumes it is a literal operator.
   void setCXXLiteralOperatorNameLoc(SourceLocation Loc) {

include/clang/AST/TemplateBase.h

public:
   }
 
   Expr *getSourceIntegralExpression() const {
     assert(Argument.getKind() == TemplateArgument::Integral);
     return LocInfo.getAsExpr();
   }
 
   NestedNameSpecifierLoc getTemplateQualifierLoc() const {
-    assert(Argument.getKind() == TemplateArgument::Template ||
-           Argument.getKind() == TemplateArgument::TemplateExpansion);
+    if (Argument.getKind() != TemplateArgument::Template &&
+        Argument.getKind() != TemplateArgument::TemplateExpansion)
+      return NestedNameSpecifierLoc();
     return LocInfo.getTemplateQualifierLoc();
   }
 
   SourceLocation getTemplateNameLoc() const {
-    assert(Argument.getKind() == TemplateArgument::Template ||
-           Argument.getKind() == TemplateArgument::TemplateExpansion);
+    if (Argument.getKind() != TemplateArgument::Template &&
+        Argument.getKind() != TemplateArgument::TemplateExpansion)
+      return SourceLocation();
     return LocInfo.getTemplateNameLoc();
   }
 
   SourceLocation getTemplateEllipsisLoc() const {
-    assert(Argument.getKind() == TemplateArgument::TemplateExpansion);
+    if (Argument.getKind() != TemplateArgument::TemplateExpansion)
+      return SourceLocation();
     return LocInfo.getTemplateEllipsisLoc();
   }
 };
 
 /// A convenient class for passing around template argument
 /// information.  Designed to be passed by reference.
 class TemplateArgumentListInfo {
   SmallVector<TemplateArgumentLoc, 8> Arguments;

lib/AST/NestedNameSpecifier.cpp

return SourceRange(TL.getBeginLoc(),
                        LoadSourceLocation(Data, Offset + sizeof(void*)));
   }
   }
 
   llvm_unreachable("Invalid NNS Kind!");
 }
 
 TypeLoc NestedNameSpecifierLoc::getTypeLoc() const {
-  assert((Qualifier->getKind() == NestedNameSpecifier::TypeSpec ||
-          Qualifier->getKind() == NestedNameSpecifier::TypeSpecWithTemplate) &&
-         "Nested-name-specifier location is not a type");
+  if (Qualifier->getKind() != NestedNameSpecifier::TypeSpec &&
+       Qualifier->getKind() != NestedNameSpecifier::TypeSpecWithTemplate)
+    return TypeLoc();
 
   // The "void*" that points at the TypeLoc data.
   unsigned Offset = getDataLength(Qualifier->getPrefix());
   void *TypeData = LoadPointer(Data, Offset);
   return TypeLoc(Qualifier->getAsType(), TypeData);
 }
 
 static void Append(char *Start, char *End, char *&Buffer, unsigned &BufferSize,