Page MenuHomePhabricator

Ensure code complete with !LoadExternal sees all local decls.
ClosedPublic

Authored by sammccall on Jan 15 2018, 9:33 AM.

Details

Summary

noload_lookups() was too lazy: in addition to avoiding external decls, it
avoided populating the lazy lookup structure for internal decls.
This is the right behavior for the existing callsite in ASTDumper, but I think
it's not a very useful default, so we populate it by default.

While here:

  • remove an unused test file accidentally added in r322371.
  • remove lookups_begin()/lookups_end() in favor of lookups().begin(), which is more common and more efficient.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Jan 15 2018, 9:33 AM
ilya-biryukov accepted this revision.Jan 16 2018, 3:15 AM

LGTM with a few NITs (see the comments)

include/clang/AST/DeclBase.h
1826 ↗(On Diff #129875)

Maybe we should consider removing the default argument and specifying it explicitly at each call site?
The difference seems subtle enough to not leave it out in the callers of the function.

include/clang/AST/DeclLookups.h
89 ↗(On Diff #129875)

Maybe move the doc to the declaration site?
Or is it common in these files to put comments at the definition?

This revision is now accepted and ready to land.Jan 16 2018, 3:15 AM
sammccall updated this revision to Diff 129937.Jan 16 2018, 4:26 AM
sammccall marked 2 inline comments as done.

Remove default value of PreserveInternalState, and move doc comment.

This revision was automatically updated to reflect the committed changes.