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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.



================
Comment at: include/clang/AST/DeclarationName.h:733
+    if (Name.getNameKind() != DeclarationName::CXXConstructorName &&
+           Name.getNameKind() != DeclarationName::CXXDestructorName &&
+           Name.getNameKind() != DeclarationName::CXXConversionFunctionName)
----------------
Formatting is incorrect here and elsewhere -- you should run the patch through clang-format.


================
Comment at: include/clang/AST/DeclarationName.h:735
+           Name.getNameKind() != DeclarationName::CXXConversionFunctionName)
+      return nullptr;
     return LocInfo.NamedType.TInfo;
----------------
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.)


================
Comment at: lib/AST/NestedNameSpecifier.cpp:465
 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))
----------------
Remove spurious parens.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56354/new/

https://reviews.llvm.org/D56354