Clang r175861 broke no_fwd_declare_std test

[GoogleCodeExporter]

I'm logging this to track information, because it was pretty hard to track down.

Clang r175861 [1] improved Clang's attribute propagation so that all 
(inheritable) attributes on a decl D are merged onto all redeclarations of D.

The no_fwd_decl_std test tries to prove that IWYU will never keep a forward 
declaration in namespace std. To do this, it forward-declares std::pair and 
#includes <utility> and then uses std::pair by name only. The intention is that 
because pair is in namespace std, a forward declare is less desirable than an 
actual #include.

However, there's a conflicting rule in VisitTagDecl (iwyu.cc@3421) [2], 
checking if a forward decl has attributes. If so, it's marked in a way that 
causes it never to be removed.

Clang's attribute change caused this to trigger for std::pair (at least on 
MSVC) because <utility> puts all declarations inside #pragma pack(push, 8), 
which generates a MaxFieldAlignmentAttr on these decls.

When attribute merging strikes, the MaxFieldAlignmentAttr on <utility>'s 
std::pair decl is propagated to the forward decl in the test.

At this point I'm trying to come up with a way forward.

I think the rule in IWYU exists so that forward declarations with more specific 
attributes than the official declarations will not be removed. That would 
change semantics.

A few options:

1) We could check if the attribute is actually written in the source code, and 
only trigger the always-keep rule for such attrs.

2) We could check if the attribute is inheritable (defined as propagated to all 
redeclarations) and only trigger the always-keep rule for non-inheritable attrs.

I'm not sure if there's a way to ask an Attr if it came into being through 
propagation or not, but that might be a third option.

[1] http://llvm.org/viewvc/llvm-project?view=revision&revision=175861
[2] 
https://code.google.com/p/include-what-you-use/source/browse/trunk/iwyu.cc#3421

Original issue reported on code.google.com by kim.gras...@gmail.com on 12 Jul 2013 at 5:12

[GoogleCodeExporter]

Here's a patch that does the third option, only set the always-keep flag if 
there are explicitly written attributes on the forward decl. Good to commit? 
Maybe the comment can be made clearer...?

Original comment by kim.gras...@gmail.com on 18 Jul 2013 at 9:31

Attachments:

• redecl-attributes.patch

[GoogleCodeExporter]

Maybe change /another declaration/another redeclaration/ but I'm not sure it 
makes the code clearer. And I think we can add a test for forward declarations 
with attributes. Attached patch has your changes too.

Original comment by vsap...@gmail.com on 19 Jul 2013 at 9:10

Attachments:

• redecl-attributes-test.patch

[GoogleCodeExporter]

Lovely! Fixed in r480.

Original comment by kim.gras...@gmail.com on 19 Jul 2013 at 4:13

• Changed state: Fixed