This is an archive of the discontinued LLVM Phabricator instance.

[Modules] Don't swallow errors when parsing optional attributes
ClosedPublic

Authored by davide on Mar 1 2016, 4:11 PM.

Details

Summary

I found this by visual inspection while trying to get up-to-speed on modules, so I'm still uncertain how to test it. If the current one is really the actual behaviour, then parseOptionalAttribute() can return void.

Diff Detail

Repository
rL LLVM

Event Timeline

davide updated this revision to Diff 49563.Mar 1 2016, 4:11 PM
davide retitled this revision from to [Modules] Don't swallow errors when parsing optional attributes.
davide updated this object.
davide added reviewers: silvas, doug.gregor.
davide updated this object.
davide added a subscriber: cfe-commits.
silvas edited edge metadata.Mar 1 2016, 4:19 PM

makes sense. can you include a testcase? I assume we are forgetting some sort of diagnostic without this patch.

Also, if learning about Clang's modules, I would start with addHeaderInclude in lib/Frontend/FrontendActions.cpp. The module map stuff ultimately boils down to that (essentially, it textually forms a header which it then more-or-less compiles as a PCH; the submodule stuff is only relevant for setting Decl::Hidden and some extra checking when importing).

davide updated this revision to Diff 49575.Mar 1 2016, 6:32 PM
davide added a reviewer: rsmith.

Added a test. Yes, with the patch we stop immediately if we fail to parse attributes, while the old code kept emitting diagnostics. I like the new behaviour better, but I'll defer the decision to you.

silvas added a comment.Mar 1 2016, 7:30 PM

This makes sense. LGTM.

This revision was automatically updated to reflect the committed changes.