search

HaveF / include-what-you-use

Automatically exported from code.google.com/p/include-what-you-use
Other
0 stars 0 forks source link

improve handling of structs #123

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
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:

GoogleCodeExporter commented 9 years ago 
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 commented 9 years ago 
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 commented 9 years ago 
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:

GoogleCodeExporter commented 9 years ago 
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 commented 9 years ago 
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 commented 9 years ago 
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 commented 9 years ago 
> (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 commented 9 years ago 
Fix looks good to me.

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

GoogleCodeExporter commented 9 years ago 
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:

GoogleCodeExporter commented 9 years ago 
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 commented 9 years ago 
> 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:

GoogleCodeExporter commented 9 years ago 
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:

GoogleCodeExporter commented 9 years ago 
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 commented 9 years ago 
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 commented 9 years ago 
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 commented 9 years ago 
Fixed in r530.

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