Prevent module import inside @implementation

[Richard Smith]

I'm not especially fond of treating annot_module_begin and
annot_module_include differently. That'll mean we get completely different
diagnostics for a '#include' in a bad place depending on whether it's
including something in the same top-level module or something in a
different top-level module.

I'm also not sure that the diagnostic change here is an improvement.
Previously:

#include "not_module.h"
#include "module.h"

... where not_module.h contains:

class X {
  // ...
// whoops, forgot the '};'

would give a "missing '}'" diagnostic, pointing at the '{' that is left
un-closed. It now gives a mysterious 'must be at the top level' diagnostic,
not pointing at the '}' any more, followed by more errors (because
'module.h' didn't get imported and because the code after the #include is
still within the class). The error here was almost certainly that a '}' was
missing, so this seems like a regression.

In what cases does this change allow us to recover from errors better?


On Wed, Feb 5, 2014 at 11:10 AM, Argyrios Kyrtzidis <kyrtzidis at apple.com>wrote:

>
> On Feb 5, 2014, at 10:48 AM, Ben Langmuir <blangmuir at apple.com> wrote:
>
> > Hi again,
> >
> > This patch grew a bit to handle a bunch of other non-top-level contexts
> that imports might show up in.  Previously annot_module_include signaled an
> end-of-module, which the parser used to stop parsing in a bunch of places,
> and then you would get unfortunate diagnostics like this when the parser
> broke out of whatever context it was in
> >
> > void foo() {
> >  #include "bar.h" // error missing }
> > } // error extraneous closing brace (})
> >
> >
> > I wasn't sure how to say 'must be at top level' in the diagnostic.  I
> only found one existing diagnostic that mentions 'top level', but several
> that refer to 'file scope', so I went with the latter.  I'm happy to change
> that if there is a convention I didn't pick up on.
>
> I think inside a namespace is also "file scope" so I prefer "top level
> scope" here.
>
> Some other nitpicks:
>
> -Watch out for 80 columns violations.
>
> -I'd prefer that we reduce the amount of passing around of the
> "llvm::PointerIntPair<Module *, 2, tok::IncludeDirectiveKind> object", e.g.
> maybe have a
>         getModuleImportAnnotationInfo
> method in Token which will return a std::pair of (Module *, DirectiveKind)
> or a wrapper struct of such info, or return the Module* directly and the
> DirectiveKind as an optional out parameter.
>
>
> >
> > Ben
> >
> > <import.patch>
> >
> > On Feb 4, 2014, at 8:48 AM, Douglas Gregor <dgregor at apple.com> wrote:
> >
> >>
> >> On Feb 4, 2014, at 8:19 AM, Ben Langmuir <blangmuir at apple.com> wrote:
> >>
> >>>
> >>> On Feb 3, 2014, at 4:51 PM, Douglas Gregor <dgregor at apple.com> wrote:
> >>>
> >>>>
> >>>> On Feb 3, 2014, at 2:32 PM, Ben Langmuir <blangmuir at apple.com> wrote:
> >>>>
> >>>>> Based on a suggestion from Jordan I've dropped the extra note, which
> will be on the same location anyway, and added that information into the
> error diagnostic.  I would have liked to say "treating #include ..."
> instead of  "treating directive ...", but after the preprocessor/lexer this
> information is lost and I don't see a nice way to pass it on to the parser.
> >>>>
> >>>> There's an ugly way to pass it on to the parser... you have a couple
> low bits in the token's annotation value to record #include vs. #import vs.
> #include_next.
> >>>
> >>> To be clear, are you suggesting making Token::PtrData a PointerIntPair?
> >>
> >> Token::PtrData is an opaque pointer with different meanings for
> different annotation token kinds. For a module import token, that could be
> PointerIntPair<Module *, 2, IncludeDirectiveKind>.
> >>
> >>      - Doug
> >>
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140205/16dfdd1f/attachment.html>