Issue 6271127710072832: No Issue - fix to be conform with std API (Closed)
|
Created:
June 11, 2015, 12:57 p.m. by sergei Modified:
June 25, 2015, 10:49 a.m. Reviewers:
Felix Dahlke CC:
Oleksandr Visibility:
Public. |
DescriptionThis code does not work with the recent compilers.
# works with MSVS 2012
Patch Set 1 #
Total comments: 1
MessagesTotal messages: 6
https://codereview.adblockplus.org/6271127710072832/diff/5629499534213120/she... File shell/src/Main.cpp (right): https://codereview.adblockplus.org/6271127710072832/diff/5629499534213120/she... shell/src/Main.cpp:39: const bool success = std::getline(std::cin, commandLine).good(); Seems standard conform the way it was to me - shouldn't this invoke operator bool() in C++ >=11? What's the error in MSVS 2012?
On 2015/06/17 19:06:52, Felix Dahlke wrote: > https://codereview.adblockplus.org/6271127710072832/diff/5629499534213120/she... > File shell/src/Main.cpp (right): > > https://codereview.adblockplus.org/6271127710072832/diff/5629499534213120/she... > shell/src/Main.cpp:39: const bool success = std::getline(std::cin, > commandLine).good(); > Seems standard conform the way it was to me - shouldn't this invoke operator > bool() in C++ >=11? What's the error in MSVS 2012? There is no error in MSVS2012, we are working now in MSVS2012. Despite it's conform with the C++11 standard there is an error in MSVS2013 because there is no `operator bool`.
On 2015/06/19 08:51:25, sergei wrote: > On 2015/06/17 19:06:52, Felix Dahlke wrote: > > > https://codereview.adblockplus.org/6271127710072832/diff/5629499534213120/she... > > File shell/src/Main.cpp (right): > > > > > https://codereview.adblockplus.org/6271127710072832/diff/5629499534213120/she... > > shell/src/Main.cpp:39: const bool success = std::getline(std::cin, > > commandLine).good(); > > Seems standard conform the way it was to me - shouldn't this invoke operator > > bool() in C++ >=11? What's the error in MSVS 2012? > > There is no error in MSVS2012, we are working now in MSVS2012. Despite it's > conform with the C++11 standard there is an error in MSVS2013 because there is > no `operator bool`. So, in a nutshell, MSVS 2013 does not implement operator bool even though it's required by the C++11 standard? I have no way of testing that, but according to the docs it should be supported: https://msdn.microsoft.com/en-us/library/ee404872.aspx Do you have a link to a bug report or something about this? This is weird...
On 2015/06/24 13:51:08, Felix Dahlke wrote: > On 2015/06/19 08:51:25, sergei wrote: > > On 2015/06/17 19:06:52, Felix Dahlke wrote: > > > > > > https://codereview.adblockplus.org/6271127710072832/diff/5629499534213120/she... > > > File shell/src/Main.cpp (right): > > > > > > > > > https://codereview.adblockplus.org/6271127710072832/diff/5629499534213120/she... > > > shell/src/Main.cpp:39: const bool success = std::getline(std::cin, > > > commandLine).good(); > > > Seems standard conform the way it was to me - shouldn't this invoke operator > > > bool() in C++ >=11? What's the error in MSVS 2012? > > > > There is no error in MSVS2012, we are working now in MSVS2012. Despite it's > > conform with the C++11 standard there is an error in MSVS2013 because there is > > no `operator bool`. > > So, in a nutshell, MSVS 2013 does not implement operator bool even though it's > required by the C++11 standard? I have no way of testing that, but according to > the docs it should be supported: > https://msdn.microsoft.com/en-us/library/ee404872.aspx > > Do you have a link to a bug report or something about this? This is weird... I have checked the code, actually there is 'explicit __CLR_OR_THIS_CALL operator bool() const', however the line still requires editing either to use `static_cast<bool>` or `.good()` or `!!` because it's marked as 'explicit'. Having that I don't think we should report about it to MS since it's not a bug, it's a feature. Despite I prefer to always use `!!` (negation of `operator!` result) here I would like to call .good() because it looks good semantically and it's not our code style (I was told already several times that we don't use `!!` for conversion to bool).
On 2015/06/24 15:12:04, sergei wrote: > I have checked the code, actually there is 'explicit __CLR_OR_THIS_CALL operator > bool() const', however the line still requires editing either to use > `static_cast<bool>` or `.good()` or `!!` because it's marked as 'explicit'. > Having that I don't think we should report about it to MS since it's not a bug, > it's a feature. Alright, now I get it - didn't know that `bool b = foo` is not invoking the explicit conversion operator. Not entirely sure why but it's the same in Clang. > Despite I prefer to always use `!!` (negation of `operator!` result) here I > would like to call .good() because it looks good semantically and it's not our > code style (I was told already several times that we don't use `!!` for > conversion to bool). Not so sure about that, we use !! a lot in JS at least, and I'm not aware of anything in our or Mozilla's code style that forbids it for C++. No strong opinion about that personally. Anyway, I agree that `good()` looks nice, either is fine by me. LGTM. 
|
||||||||||||||||||||||||||||||||||||||||||
