improve handling of structs

[GoogleCodeExporter]

While trying to improve a header file that contained the line

int try(struct mystr *f);

IWYU said I did not need the header file that contains the definition of struct 
mystr.
However if I remove that header file I get a warning.
tst4.c in the attachment is the one with the include, tst5 without.

Results:
frans@2600-m4:/home/home/frans/clang/FMtst$ include-what-you-use  tst4.c

tst4.c should add these lines:

tst4.c should remove these lines:
- #include "mystr.h"  // lines 1-1

The full include-list for tst4.c:

---
frans@2600-m4:/home/home/frans/clang/FMtst$ include-what-you-use  tst5.c
tst5.c:1:16: warning: declaration of 'struct mystr' will not be visible outside 
of this function [-Wvisibility]
int try(struct mystr *f);
               ^

(tst5.c has correct #includes/fwd-decls)

Personally I think I'd prefer to have the header file for mystr included (and 
not have the warning.

This is on ubuntu 12.04 with an svn checkout of feb 8, 2014

Original issue reported on code.google.com by fransmeu...@gmail.com on 9 Feb 2014 at 12:27

Attachments:

• iwyu.struct.tar.gz

[GoogleCodeExporter]

Thanks! As far as I can tell, this only manifests in C code (the warning in 
Clang is only thrown in C mode). I'll take a look and see if we can make IWYU's 
treat inline declarations as forward-declarations in C++ only.

Original comment by kim.gras...@gmail.com on 9 Feb 2014 at 12:58

[GoogleCodeExporter]

That would be very nice! Let me know if you need a tester!

Original comment by fransmeu...@gmail.com on 10 Feb 2014 at 2:51

[GoogleCodeExporter]

Attached is a bare patch along those lines.

If this works for you I'd like to think about a test infrastructure for C code 
(the current stuff in tests/ only runs tests for .cc files), but that will take 
some more effort.

Original comment by kim.gras...@gmail.com on 10 Feb 2014 at 8:40

Attachments:

• issue123.patch

[GoogleCodeExporter]

Thanks for the quick response.
Your patch works for me.
For the test program I submitted it gives:

frans@2600-m4:/home/home/frans/clang/FMtst$ ./iwyu -I. tst5.c

tst5.c should add these lines:
struct mystr;

tst5.c should remove these lines:
- #include <mystr.h>  // lines 1-1

The full include-list for tst5.c:
struct mystr;

I am not sure if I want to keep the .h file in this case, but with this patch I 
get the struct, so that is good, and if desired I can simply replace it back 
with the include with a small sed cmd or so.

Original comment by fransmeu...@gmail.com on 12 Feb 2014 at 7:53

[GoogleCodeExporter]

Good to hear, thanks!

Yes, since you only use mystr in name, IWYU will suggest a forward-decl instead 
of an #include. Had you used the full type, the #include would stay (I tried 
that here.)

I'll try and get tests in place for C code separately, and then commit this 
change or a variation on it.

Original comment by kim.gras...@gmail.com on 12 Feb 2014 at 8:07

[GoogleCodeExporter]

Small minor addition.

Currently I get:
tst.h should add these lines:
struct mystr;

You might consider adding a comment stating where this comes from.
e.g.
struct mystr; /* from mystr.h */

Or something along those lines (or am I goldplating ;-) )

Original comment by fransmeu...@gmail.com on 12 Feb 2014 at 8:21

[GoogleCodeExporter]

> (or am I goldplating ;-) )

Yes :-)

We don't necessarily know where mystr is defined, the forward declaration will 
be suggested even if you don't have #include "mystr.h" before-hand.

Original comment by kim.gras...@gmail.com on 12 Feb 2014 at 8:38

[GoogleCodeExporter]

Fix looks good to me.

Original comment by vsap...@gmail.com on 12 Feb 2014 at 10:30

[GoogleCodeExporter]

I could use a second pair of eyes on the patch for the test runner, it turned 
out larger than I had hoped.

It's a bit of a weird design that generates new testcase classes at run-time, 
but it seems it's built that way to click into the Python unittest module. I 
had to make it even more dynamic to allow creation of different testcase types 
depending on language.

It runs all the tests correctly, and it picks up the new C tests from tests/c 
as expected. Once this is in place, I can fix this issue with test coverage.

Thanks!

Original comment by kim.gras...@gmail.com on 23 Feb 2014 at 7:50

Attachments:

• run_iwyu_tests_c.patch

[GoogleCodeExporter]

At the first glance I don't like parallel similar support for C and C++. For 
example, RegisterFilesForTesting knows which directory, extension, and class to 
use. But class itself knows which directory and extension to use. I am not 
sure, but using separate Python classes for C and C++ tests seems to be 
redundant. At least if we store additional flags in the same class for C and 
C++ tests, there should be no name clashes because we use filenames as 
dictionary keys and files have different extensions.

Nitpicking: looks like module 'logging' was intended to be used immediately 
after its import, but now there is 'fnmatch' between import and usage.

Unrelated: looks like class Flags is unused. Can be removed in a separate 
commit.

Original comment by vsap...@gmail.com on 24 Feb 2014 at 9:56

[GoogleCodeExporter]

> using separate Python classes for C and C++ tests seems to be redundant

I thought so too, at first, but the constructor is part of the contract for 
unittest.TestCase-derivatives, so there's no way to parameterize the 
constructor. That's why I needed separate types, to have something to associate 
dir and extension with.

Attached is a variation where the root dir and pattern are stuck on each test 
class, and RegisterFilesForTesting pulls them off the test class. It removes 
some duplication, at least, even if there's still multiple classes in flight.

> At least if we store additional flags in the same class for 
> C and C++ tests, there should be no name clashes because we
> use filenames as dictionary keys and files have different
> extensions.

Yes, the flags _could_ be hoisted into the base class. The derived classes are 
required for keeping track of root dir, pattern (used for finding associated 
files) anyway, so I figured it made more sense to keep flags per language.

> Nitpicking: looks like module 'logging' was intended to be used
> immediately after its import, but now there is 'fnmatch' between
> import and usage.

Oops, right you are. I moved the fnmatch import down.

> Unrelated: looks like class Flags is unused. Can be removed in a separate 
commit.

Cool, gotten rid of in r528

Original comment by kim.gras...@gmail.com on 25 Feb 2014 at 8:07

Attachments:

• run_iwyu_tests-2.patch

[GoogleCodeExporter]

I've thought what if we add 'rootdir' and 'pattern' to class when create it in 
run-time? Corresponding patch is attached.

Original comment by vsap...@gmail.com on 27 Feb 2014 at 4:37

Attachments:

• run_iwyu_tests-3.patch

[GoogleCodeExporter]

Nice! This is the first I see of generating types at run-time, so it didn't 
occur to me we could add class-level properties as well as methods.

Feel free to commit. If you don't, I'll take the liberty to commit it later 
tonight, so I can move forward with closing this issue.

Thanks!

Original comment by kim.gras...@gmail.com on 27 Feb 2014 at 4:47

[GoogleCodeExporter]

Please review comments and doc strings. I've updated some of them, but haven't 
reviewed all.

Original comment by vsap...@gmail.com on 27 Feb 2014 at 5:32

[GoogleCodeExporter]

Thanks, I made a couple of micro-edits (removed inconsistent whitespace and 
changed the comment from "add methods" to "add attrs") but otherwise committed 
as-was in r529.

I'll try to commit the actual fix for this issue now.

Original comment by kim.gras...@gmail.com on 27 Feb 2014 at 7:13

[GoogleCodeExporter]

Fixed in r530.

Original comment by kim.gras...@gmail.com on 27 Feb 2014 at 8:09

• Changed state: Fixed