<div dir="ltr">Thumbs up for your first patch, it's probably best if you start an official review process using Phabricator, make sure you CC someone familiar with this code.</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 28, 2015 at 10:30 AM, dyp <span dir="ltr"><<a href="mailto:dyp-cpp@gmx.net" target="_blank">dyp-cpp@gmx.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Dear clang developers,<br>
<br>
I've been trying to implement the resolution of C++ CWG 1423 in clang++<br>
which restricts conversions from nullptr_t to bool to<br>
direct-initialization. This is my first attempt at contributing to<br>
clang, and as far as I can tell, it's the only standard conversion that<br>
is not an implicit conversion.<br>
<br>
(I couldn't find any bug report nor mailing on this, and I wanted to<br>
contribute to clang for some time now, so I just went ahead and<br>
implemented it. Even if my solution is too crappy to be accepted, it was<br>
a nice exercise and not too hard I guess ;)<br>
<br>
While I was successful in adding the restriction, there remain some<br>
issues with my fix, and I'm not very comfortable with my<br>
"solution" for list-initialization. Therefore, I'd appreciate any<br>
feedback and suggestions how to introduce the restrictions. I hope my<br>
request is appropriate for this mailing list.<br>
<br>
<br>
Specifically, I have the following questions:<br>
<br>
1. Changes of public member functions<br>
I've modified the signature of two public member functions of the Sema<br>
class by adding an additional parameter. How problematic is this change?<br>
(API etc)<br>
<br>
2. OpenMP support<br>
I've added an additional parameter to the first two overloads of<br>
Sema::PerformImplicitConversion, that specifies whether or not we're<br>
performing a direct initialization. This function is also used by<br>
SemaOpenMP.cpp<br>
I have never worked with OpenMP, so I don't really know if the usages in<br>
SemaOpenMP.cpp occur within a direct-initialization context.<br>
<br>
3. Diagnostics<br>
How important are good diagnostic messages for this new error case?<br>
Should better/specific diagnostics be included in the patch, or can they<br>
be improved in a later patch?<br>
<br>
4. Test cases<br>
Since CWG 1423 is classified as a DR, and g++ implements it also in<br>
C++11 mode, I did not include any language standard switches for this<br>
fix. However, this leads to some of the existing tests now failing. I<br>
have modified them, and added further test cases for this special<br>
implicit conversion. Unfortunately, this means that there's no separate<br>
test for this fix. Is there a better solution?<br>
<br>
5. Support for list-initialization<br>
I couldn't figure out how to properly add a distinction between<br>
direct-list-initialization and copy-list-initialization for CWG 1423.<br>
The TryListInitialization function relies on the InitListChecker class<br>
for all scalar initialization. As the comment some lines above points<br>
out, the InitListChecker class always performs copy-initialization, and<br>
it seemed to me to be deeply ingrained in its algorithms. There already<br>
are several special cases in TryListInitialization that handle<br>
direct-list-initialization where the destination type is a record type.<br>
I've tried to add scalar initialization to one case that uses<br>
InitializationSequence::InitializeFrom (which finally dispatches to<br>
Sema::TryImplicitConversion which I've already modified for<br>
direct-nonlist-initialization), but this makes some tests fail<br>
apparently due to a lack of support of initialization of vector types,<br>
different diagnostics and compound literals.<br>
It seems this part could benefit from some refactoring, although I'm not<br>
sure what would be a good approach.<br>
As a quick and dirty fix, I've routed only the nullptr_t -> bool<br>
conversion through this special case, which is of course only a<br>
workaround to prevent the aforementioned issues. It does work, but it<br>
feels like a really dirty hack.<br>
<br>
6. A naming issue<br>
It occurred to me when writing this email that my extension of<br>
Sema::TryImplicitConversion might be inappropriate:<br>
Direct-initialization is not an implicit conversion as defined in<br>
C++14[conv]p3.<br>
Sema::TryContextuallyConvertToBool also uses TryImplicitConversion, even<br>
though that isn't strictly an implicit conversion either. (Probably just<br>
a bike-shedding issue.)<br>
<br>
<br>
Thank you very much and kind regards,<br>
<br>
dyp<br>
<br>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div>