enum declarations are treated as useless

[GoogleCodeExporter]

    //An example:
    // myenum.hh
    enum foo_t {
        FOO_A,
        FOO_B,
    };
    // myfun.hh
    #include "myenum.hh"
    int from_enum (enum foo_t v);

iwyu misreports declaration:

    $ include-what-you-use myfun.hh 

    myfun.hh should remove these lines:
    - #include "myenum.hh"  // lines 2-2

If you follow it's advice the header will be unusable:

    // myfun.hh
    //#include "myenum.hh"
    int from_enum (enum foo_t v);

    $ gcc -c myfun.hh 
    myfun.hh:3:21: error: use of enum 'foo_t' without previous declaration
     int from_enum (enum foo_t v);

Which makes sense as enum constants can influence on enum's signedness and even 
size (int / long).

Thanks!

Original issue reported on code.google.com by slyich on 18 Sep 2013 at 2:25

[GoogleCodeExporter]

Thanks, clean repro.

I wonder if this has anything to do with the fact that you're running IWYU on a 
header file (not a source file), and it fails to evaluate the declaration 
without a definition.

Is there a myfun.cc you could try and see if the problem persists? Running IWYU 
on x.cc will also process x.hh, so both should be covered in that case.

Original comment by kim.gras...@gmail.com on 18 Sep 2013 at 7:08

[GoogleCodeExporter]

I've originally found an error on .cc file.
.hh was to make shorter testcase for you.

The problem will go away if you drop 'enum' (make header more of C++ style)
    int from_enum (enum foo_t v);
to
    int from_enum (foo_t v);

but I kept 'enum' for header be compatible with C and a bit more clear when 
reading.

Original comment by slyich on 18 Sep 2013 at 7:42

[GoogleCodeExporter]

Oh, that detail is probably important. My guess is the 'enum' prefix does 
something to the AST that IWYU doesn't expect.

Thanks, I'll try and look into it before too long.

Original comment by kim.gras...@gmail.com on 18 Sep 2013 at 7:58

[GoogleCodeExporter]

It turns out C++ accepts in-line forward declarations for classes, structs and 
unions, like so:

  void foo(class Bar* b);

A typename prefixed by its kind is called an elaborated type. Another kind of 
elaborated type is, you guessed it, the enum. However, enums can't be 
forward-declared (in C++98) and so the elaborated type syntax doesn't have the 
same implications for an enum.

IWYU handles elaborated types in declarations, but missed the fact that enums 
can be declared the same way.

The attached patch adds a check for enum types to counter this behavior. It 
also adds a separate test case for elaborated types, and removes the 
corresponding elaborated tests from badinc.cc.

@slyich, do you have Clang/LLVM/IWYU set up so you can build with this patch 
applied? I'd love confirmation that this solves your problem.

@vsapsai, could you review the patch?

Thanks!

Original comment by kim.gras...@gmail.com on 19 Sep 2013 at 7:54

Attachments:

• elaboration.patch

[GoogleCodeExporter]

> @slyich, do you have Clang/LLVM/IWYU set up so you can build with this patch 
applied? I'd love confirmation that this solves your problem.

Yeah. Just applied on top of -3.3 version.
Works like a charm! The warning gone away.

Thank you!

Original comment by slyich on 19 Sep 2013 at 8:30

[GoogleCodeExporter]

Thanks for confirming!

Attached is an updated patch, I changed the test names from elaboration* to 
elaborated_type* and simplified the one-line code change.

Original comment by kim.gras...@gmail.com on 23 Sep 2013 at 8:24

Attachments:

• elaborated_type.patch

[GoogleCodeExporter]

Kim, you've tuned elaborated EnumType, but what if we make enums not 
forward-declarable (the patch is attached)? The rest of changes are fine.

FWIW, CanForwardDeclareType returned true for EnumType because in 
VisitFunctionDecl we've set 
current_ast_node()->set_in_forward_declare_context(true). AST nodes inherit 
in_forward_declare_context from parent nodes, that's how EnumType ended up in 
forward declare context, which caused CanForwardDeclareType to return true.

Original comment by vsap...@gmail.com on 1 Oct 2013 at 3:29

Attachments:

• enum_cannot_forward_declare.patch

[GoogleCodeExporter]

Hmm, that seems even more correct.

My change causes VisitTagType not to report enums at all. Presumably they are 
picked up by some other visitor and reported as full uses.

Your change short-circuits what I did since my check for EnumType is inside the 
CanForwardDeclareType condition, and causes VisitTagType to report a full use.

So if we apply your change, we'll never need to consider enums for elaborated 
types.

I'll apply locally and update the patch. I'll try to get rid of the 
svn:eol-style properties as well, they're not supposed to be there.

Original comment by kim.gras...@gmail.com on 1 Oct 2013 at 7:43

[GoogleCodeExporter]

Here's a new patch with your change, my tests and some additional commentary.

This should be clean to commit, but please take another look.

Original comment by kim.gras...@gmail.com on 5 Oct 2013 at 7:32

Attachments:

• elaborated_type2.patch

[GoogleCodeExporter]

Shouldn't it be
  void namespace_qualified(class Elaboration::Class* c);
in elaborated_type.cc? Elaboration::Class is already elaborated without "class" 
keyword, but I've expected elaboration with keyword like in previous examples. 
Kim, what do you think?

And I have a few remarks regarding formatting. I'm not in favor of changing 
formatting unless it improves readability. And in VisitTagType it harms 
readability a little bit. Earlier it was

// comment 1
code related to comment 1

// comment 2
code related to comment 2

and now extra empty lines make comment-code blocks not so apparent.

Original comment by vsap...@gmail.com on 6 Oct 2013 at 7:09

[GoogleCodeExporter]

Thanks for reviewing.

I wonder if that's even valid syntax... I know Clang doesn't accept top-level 
forward declarations like this:

  class Elaboration::Class;

so it's weird that it should accept it inline as an elaborated type. But I 
can't make out the intent of the standard text for elaborated types.

I wanted the test to demonstrate that even if Elaboration::Class is by 
definition an elaborated type, it still needs a forward declaration.

Maybe I should ask on the Clang list if the current behavior is correct?

Reverted formatting changes, I'll post a new patch when/if I hear anything from 
cfe-dev.

Original comment by kim.gras...@gmail.com on 7 Oct 2013 at 6:47

[GoogleCodeExporter]

OK, let's leave
  void namespace_qualified(Elaboration::Class* c);
It tests the case of elaborated type with namespace qualifier and that's what's 
important.

I don't know if it's valid syntax, at least Clang doesn't complaint about it. 
On one hand "class Elaboration::Class*" doesn't add anything to 
"Elaboration::Class*", but on the other hand why not to allow such syntax.

Original comment by vsap...@gmail.com on 7 Oct 2013 at 5:39

[GoogleCodeExporter]

OK, thanks. Committed in r487.

I never managed to form a coherent question around elaborated types for 
cfe-dev. Maybe I will soon :-)

Original comment by kim.gras...@gmail.com on 7 Oct 2013 at 7:20

• Changed state: Fixed

[GoogleCodeExporter]

Kim, can you please fix Windows line endings in tests/elaborated_type*?

Original comment by vsap...@gmail.com on 8 Oct 2013 at 7:49

[GoogleCodeExporter]

Ugh, I thought I had :-(

My SVN client is set to eol-style native system-wide (work policy), so I'm not 
sure I can override it.

Can you check if my commit of elaborated_type.cc in r488 looks OK? If so I can 
do the others one by one.

Thanks for noticing!

Original comment by kim.gras...@gmail.com on 8 Oct 2013 at 7:59

[GoogleCodeExporter]

r488 fixes line endings in elaborated_type.cc.

Original comment by vsap...@gmail.com on 8 Oct 2013 at 8:12