Test template_args fails on MSVC

[GoogleCodeExporter]

What steps will reproduce the problem?
1. $ python run_iwyu_tests template_args

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

 Expected: success.

 Actual: AssertionError: tests/template_args.cc:18: Unexpected diagnostic:
  IndirectClass is defined in "tests/indirect.h", which isn't directly
  #included (for autocast).

This is because all classes are erroneously classified as having a conversion 
constructor.

For some reason an implicit move ctor is generated, which tripped up our 
conversion ctor detection. I thought IWYU ran in C++03 mode?

Attached patch do not treat implicit move ctors as conversion ctors. Two more 
tests pass for MSVC, I haven't been able to try other platforms.

Original issue reported on code.google.com by kim.gras...@gmail.com on 1 Jan 2013 at 10:29

Attachments:

• conversionctor.patch

[GoogleCodeExporter]

Which LLVM/Clang revision? For me at r171351 test template_args passes. On my 
machine IWYU runs in C++03 mode and complains about C++11 expressions.

Original comment by vsap...@gmail.com on 2 Jan 2013 at 9:44

[GoogleCodeExporter]

Clang/LLVM r171027. There are consistent test failures since forever with MSVC 
on Windows, so I don't think it's a new thing (though I'm not 100% sure that 
this specific test has failed before.)

It could be a Clang/Windows quirk that move constructors are generated in C++03 
mode. Either way, the patch makes HasImplicitConversionCtor C++11-aware.

Original comment by kim.gras...@gmail.com on 2 Jan 2013 at 9:50

[GoogleCodeExporter]

I see diagnostic
IndirectClass is defined in "tests/indirect.h", which isn't directly #included 
(for autocast).
when I run IWYU with -std=c++11.

Kim, can you, please, add a test case for implicit move constructor which we 
can run explicitly in C++11 mode?

Original comment by vsap...@gmail.com on 7 Jan 2013 at 7:30

[GoogleCodeExporter]

I don't know if we handle implicit move constructors generally. This patch only 
exempts them from being classified as a conversion constructor, and I think 
that's already covered by implicit_ctor.cc.

Or what did you have in mind?

Original comment by kim.gras...@gmail.com on 7 Jan 2013 at 7:56

[GoogleCodeExporter]

Also, that's the same diagnostic I'm seeing. Are you saying that doesn't make 
the template_args test fail for you?

Original comment by kim.gras...@gmail.com on 7 Jan 2013 at 7:57

[GoogleCodeExporter]

OK, I think I understand what you're getting at -- you want an isolated test 
case we can run with -std=c++11 that fails without the patch and succeeds with 
it.

Makes sense, attached is a minimal case based on template_args. I set it up to 
run with -std=c++11.

Let's hope this behaves the same on your machine :-)

Original comment by kim.gras...@gmail.com on 7 Jan 2013 at 9:35

Attachments:

• conversionctor.patch

[GoogleCodeExporter]

For the record, this test with the patch applied also works on Ubuntu/GCC.

Original comment by kim.gras...@gmail.com on 10 Jan 2013 at 12:34

[GoogleCodeExporter]

And to close the loop, this came up on cfe-commits today:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130107/071468.html

  "[...] I believe -std=c++11 is added by the driver on windows [...]"

I checked and found this in Driver/Tools.cpp line 2349 where "-std=c++11" is 
added as an arg if the code runs on Windows.

In off-list correspondence with the author he said this was added because 
MSVC's cl.exe defaults to C++11 and because some Windows headers assume C++11.

I've also confirmed that adding "-std=c++98" to the command line for 
template_args.cc makes the test succeed.

I still think we should apply the patch -- our analysis really doesn't care if 
this is C++11 or not. If we ever run into a situation where there's a conflict 
between C++11 and earlier standard revs, we can condition behavior on the 
LangOptions::CPlusPlus0x (now called CPlusPlus11) flag, but for this specific 
case we either find a move ctor and skip past it or we don't find it at all.

Original comment by kim.gras...@gmail.com on 11 Jan 2013 at 9:07

[GoogleCodeExporter]

Committed the patch. But I still don't understand why move constructor is 
mentioned at all for the code

char Foo(IndirectClass);
IndirectClass ic;

Original comment by vsap...@gmail.com on 13 Jan 2013 at 7:04

[GoogleCodeExporter]

Thank you!

Neither did I, until I took a few hours to spelunk... Here's my current 
understanding:

- Indirect does not have a written constructor of any kind
- Implicit constructors (and other implicit methods) are generated on-demand, 
in this case when the global IndirectClass instance ic is created.
- The presence of a function declaration accepting an IndirectClass by value 
triggers the logic for "autocast" detection, and this is where the implicitly 
generated move constructor is incorrectly classified as a conversion 
constructor.

Now that I got to the bottom of that, I realize how uninformative the test case 
was. Attached is a revised test case with comments trying to describe the above.

Original comment by kim.gras...@gmail.com on 13 Jan 2013 at 10:49

Attachments:

• conversion_ctor_test.patch

[GoogleCodeExporter]

Now it makes sense, comments are really helpful.

Feel free to commit, but pay attention to line endings.

Original comment by vsap...@gmail.com on 20 Jan 2013 at 6:00

[GoogleCodeExporter]

Thanks, fixed in r435. Extra attention to line endings paid :-)

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

• Changed state: Fixed