This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by steveire on Jan 5 2019, 5:12 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

steveire created this revision.Jan 5 2019, 5:12 AM
lebedev.ri retitled this revision from Replace asserts with if() in SourceLocation accessors to [AST] Replace asserts with if() in SourceLocation accessors.Jan 5 2019, 5:41 AM

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 ↗(On Diff #180369)

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

735 ↗(On Diff #180369)

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 ↗(On Diff #180369)

Remove spurious parens.

steveire updated this revision to Diff 180548.Jan 7 2019, 12:47 PM

Fix formatting

steveire marked an inline comment as done.Jan 7 2019, 12:50 PM
steveire added inline comments.
include/clang/AST/DeclarationName.h
735 ↗(On Diff #180369)

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 accepted this revision.Jan 7 2019, 1:11 PM

LGTM aside from a formatting nit.

include/clang/AST/DeclarationName.h
735 ↗(On Diff #180369)

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 ↗(On Diff #180369)

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

This revision is now accepted and ready to land.Jan 7 2019, 1:11 PM
This revision was automatically updated to reflect the committed changes.
steveire marked an inline comment as done.Jan 7 2019, 2:21 PM
steveire added inline comments.
lib/AST/NestedNameSpecifier.cpp
465 ↗(On Diff #180369)

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