search

HaveF / include-what-you-use

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

#pragma once interferes with #include ordering #165

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?  Give the *exact* arguments passed
to include-what-you-use, and attach the input source file that gives the
problem (minimal test-cases are much appreciated!)
1. Create a header file with missing includes such that iwyu will make suggest 
additional headers.
2. Ensure the header utilizes #pragma once, which must be the first non comment 
directive.  Header may also contain include guards.
3. Run iwyu and fix_includes.py.  New includes will be added prior to the 
pragma once and existing include guards.

What is the expected output? What do you see instead?

Expect new includes to be added after pragma once and withing the include 
guards, if any.

What version of the product are you using? On what operating system?

3.5, Ubuntu 14.10

Please provide any additional information below.

Patch attached that I believe addresses the issue.

Original issue reported on code.google.com by c.d.glo...@gmail.com on 30 Dec 2014 at 4:35

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks! Patch looks nominally good, I haven't looked into how fix_includes.py 
works more deeply.

Could you also add a test case for this in fix_includes_test.py?

Original comment by kim.gras...@gmail.com on 30 Dec 2014 at 8:55

GoogleCodeExporter commented 9 years ago 
> +_PRAGMA_LINE_RE = re.compile(r'\s*#pragma')

Think this should try to catch all of "#pragma once", e.g. with 
r'\s*#pragma\s+once'.

> +        file_lines[i].type not in [_COMMENT_LINE_RE, _BLANK_LINE_RE, 
_PRAGMA_LINE_RE]):

Please wrap this at 80 columns.

-                          _HEADER_GUARD_RE, _HEADER_GUARD_DEFINE_RE):
+                          _HEADER_GUARD_RE, _HEADER_GUARD_DEFINE_RE, 
_PRAGMA_LINE_RE):

Please wrap at 80 columns.

With these things fixed and a test added, I think this looks great.

Original comment by kim.gras...@gmail.com on 30 Dec 2014 at 9:17

GoogleCodeExporter commented 9 years ago 

>Think this should try to catch all of "#pragma once", e.g. with 
>r'\s*#pragma\s+once'.

Yes, I was on the fence about this, but I agree now.

> Please wrap this at 80 columns.

OK

> With these things fixed and a test added, I think this looks great.

I'll post a new patch with the changes.

Original comment by c.d.glo...@gmail.com on 30 Dec 2014 at 1:44

GoogleCodeExporter commented 9 years ago 
New patch.
- Respected 80 char limit
- Changed regex to catch #pragma once only instead of all pragmas
- Added 4 tests
  = Check for traditional #pragma once immediately preceding header guards.
  = Check solo #pragma once (no header guard)
  = Check special case variants of each where #pragma once is separated from header guard via blanks or comments.

I had to make a couple of changes to the patch to get the solo #pragma once to 
work.

Original comment by c.d.glo...@gmail.com on 30 Dec 2014 at 4:25

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks, this looks good to me. I made some small edits to remove whitespace and 
unrelated changes. I'd like Volodymyr to take a look too, but after that I'd be 
happy to commit this.

Many thanks for the patch!

Original comment by kim.gras...@gmail.com on 30 Dec 2014 at 9:23

Attachments:

GoogleCodeExporter commented 9 years ago 
Does "# pragma once" (space between # and pragma) work like "#pragma once"? The 
rest looks good. Thanks for the great job, Chris and Kim.

Original comment by vsap...@gmail.com on 3 Jan 2015 at 11:50

GoogleCodeExporter commented 9 years ago 
Good catch. There could be arbitrary space in there so the regex should be 
\s*#\s*pragma\s+once to handle that.

Original comment by c.d.glo...@gmail.com on 3 Jan 2015 at 11:57

GoogleCodeExporter commented 9 years ago 
And I think we can add one more test - #pragma which is not #pragma once 
without header guards.

Original comment by vsap...@gmail.com on 4 Jan 2015 at 5:22

GoogleCodeExporter commented 9 years ago 
Good input! I'm going out of town for a couple of days, unless one of you beat 
me to it, I can try to fix this when I get back.

Original comment by kim.gras...@gmail.com on 4 Jan 2015 at 6:58

GoogleCodeExporter commented 9 years ago 
Fixed in r597. Sorry this took so long!

Original comment by kim.gras...@gmail.com on 30 Jan 2015 at 8:28