[PATCH] Begin to implement the C++0x narrowing error

This patch implements as much of the narrowing conversion error specified by [dcl.init.list] as is possible without generalized initializer lists or full constant expression support, and emits a c++0x-compat warning in C++98 mode.

I haven't done much with this code before, so let me know if there are better ways to do pieces of this, or if I've attached the logic in completely the wrong place.

The FixIt currently uses a typedef's basename without qualification, which is likely to be incorrect on some code. I'm not sure I'll need anything more correct to clean up Google's codebase, but if you want me to fix it up I'd like to do that in a separate change.

The "has the same value after a round-trip through another type" logic is messier than I'd like, but cleaning it up appears to need more methods on types like APInt and APFloat, which I'd like to postpone until another change too. If I'm just missing existing facilities, please point them out.

The warning is currently off by default. I'll fix LLVM and clang before turning it on.

[Jeffrey Yasskin, 2011-07-14 04:23:24]

Diff at http://codereview.appspot.com/download/issue4698047_7001.diff, but the
up to date version will always be at http://codereview.appspot.com/4698047.

[Jeffrey Yasskin (google), 2011-07-18 16:55:21]

I've attached the latest version of this patch.

On Wed, Jul 13, 2011 at 9:23 PM,  <jyasskin@gmail.com> wrote:
> Reviewers: ,
>
> Message:
> Diff at http://codereview.appspot.com/download/issue4698047_7001.diff,
> but the up to date version will always be at
> http://codereview.appspot.com/4698047.
>
> Description:
> This patch implements as much of the narrowing conversion error
> specified by [dcl.init.list] as is possible without generalized
> initializer lists or full constant expression support, and emits a
> c++0x-compat warning in C++98 mode.
>
> I haven't done much with this code before, so let me know if there are
> better ways to do pieces of this, or if I've attached the logic in
> completely the wrong place.
>
> The FixIt currently uses a typedef's basename without qualification,
> which is likely to be incorrect on some code. I'm not sure I'll need
> anything more correct to clean up Google's codebase, but if you want me
> to fix it up I'd like to do that in a separate change.
>
> The "has the same value after a round-trip through another type" logic
> is messier than I'd like, but cleaning it up appears to need more
> methods on types like APInt and APFloat, which I'd like to postpone
> until another change too. If I'm just missing existing facilities,
> please point them out.
>
> Because I named the CXX0xCompat DiagGroup, I migrated all the existing
> c++-compat warnings to it, which fixed the names of two of the warnings.
>
> The warning is on by default, but it's quite noisy on existing C++98
> codebases. Let me know if it should be off by default.
>
> The diagnostics for constants that can't be narrowed include strings
> like "Int: 999" for APValues. We should fix this by adding an
> operator<<(Diag, APValue) overload, but again, in another change.
>
> Please review this at http://codereview.appspot.com/4698047/
>
> Affected files:
>   M     include/clang/Basic/DiagnosticGroups.td
>   M     include/clang/Basic/DiagnosticSemaKinds.td
>   M     include/clang/Sema/Initialization.h
>   M     include/clang/Sema/Sema.h
>   M     lib/Sema/SemaInit.cpp
>   A     test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
>   M     test/Parser/objc-init.m
>   M     test/SemaTemplate/instantiate-c99.cpp
>   M     test/SemaTemplate/instantiate-expr-4.cpp
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>

[scshunt, 2011-07-21 23:46:34]

Looks good, mostly. A few comments.

http://codereview.appspot.com/4698047/diff/30001/include/clang/Basic/Diagnost...
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/4698047/diff/30001/include/clang/Basic/Diagnost...
include/clang/Basic/DiagnosticSemaKinds.td:2472: "override this error by
inserting an explicit cast">;
s/error/message/ for when it's a warning?

http://codereview.appspot.com/4698047/diff/30001/lib/Sema/SemaInit.cpp
File lib/Sema/SemaInit.cpp (right):

http://codereview.appspot.com/4698047/diff/30001/lib/Sema/SemaInit.cpp#newcod...
lib/Sema/SemaInit.cpp:5163: OS << BT->getName(S.getLangOptions());
Are template parameters TypedefTypes? An assert here that one of the branches
was taken would be nice.

http://codereview.appspot.com/4698047/diff/30001/test/CXX/dcl.decl/dcl.init/d...
File test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp (right):

http://codereview.appspot.com/4698047/diff/30001/test/CXX/dcl.decl/dcl.init/d...
test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp:4: // mode.
I don't see any tests for the fixit anywhere; those would be useful.

http://codereview.appspot.com/4698047/diff/30001/test/CXX/dcl.decl/dcl.init/d...
test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp:36: // * from a
floating-point type to an integer type, or
Testing for non-constant floats would probably be good.

[scshunt, 2011-07-22 00:03:08]

Looks pretty good; a few comments and then I'd say it's ready to go.

[Jeffrey Yasskin, 2011-07-22 19:43:23]

http://codereview.appspot.com/4698047/diff/30001/include/clang/Basic/Diagnost...
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/4698047/diff/30001/include/clang/Basic/Diagnost...
include/clang/Basic/DiagnosticSemaKinds.td:2472: "override this error by
inserting an explicit cast">;
On 2011/07/21 23:46:35, scshunt wrote:
> s/error/message/ for when it's a warning?

Done.

http://codereview.appspot.com/4698047/diff/30001/lib/Sema/SemaInit.cpp
File lib/Sema/SemaInit.cpp (right):

http://codereview.appspot.com/4698047/diff/30001/lib/Sema/SemaInit.cpp#newcod...
lib/Sema/SemaInit.cpp:5163: OS << BT->getName(S.getLangOptions());
On 2011/07/21 23:46:35, scshunt wrote:
> Are template parameters TypedefTypes? An assert here that one of the branches
> was taken would be nice.

Template parameters wind up substituted here, so you get a cast to the
underlying type, not the symbolic type. But clang records the original type
parameter's spelling, so I've added a branch to catch that, and a branch to just
return in case we the target type wasn't a builtin for some reason.

... Or not. That breaks fixits for MyTpl<char>, where the char member also has a
substituted template type, and there's no easy way to tell at which level the
type was substituted.

Everything that can be narrowed to shows up as a BuiltinType anyway, but I've
left the else just in case.

http://codereview.appspot.com/4698047/diff/30001/test/CXX/dcl.decl/dcl.init/d...
File test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp (right):

http://codereview.appspot.com/4698047/diff/30001/test/CXX/dcl.decl/dcl.init/d...
test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp:4: // mode.
On 2011/07/21 23:46:35, scshunt wrote:
> I don't see any tests for the fixit anywhere; those would be useful.

I've added p7-0x-fixits.cpp, and that discovered that my new
SubstTemplateTypeParmType branch was breaking the fixits for initializations of
templates. :(

http://codereview.appspot.com/4698047/diff/30001/test/CXX/dcl.decl/dcl.init/d...
test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp:36: // * from a
floating-point type to an integer type, or
On 2011/07/21 23:46:35, scshunt wrote:
> Testing for non-constant floats would probably be good.

The code doesn't check whether floats in float->int conversions are constant or
non-constant. But ok.