Re: position.hh compile error C4146 (VisualStudio 2017)
> On 18 Aug 2018, at 17:03, Akim Demaille wrote: > > I don’t remember why I wrote it this way, but today I don’t care much about > INT_MIN here. I think the code has become too complex for what it meant to > do. Also, maybe I should have sticked to int instead of trying to unsigned, > but it’s too late to change that :) Maybe std::fpos and std::streamoff could be used, which are guaranteed to be able to represent the maximum file size.
Re: position.hh compile error C4146 (VisualStudio 2017)
Works for me. (Not that I tried it or anything :-) I don't have Visual
Studio anywhere.)
2018-08-18 10:33 GMT-05:00 Akim Demaille :
>
> > Le 18 août 2018 à 17:10, Rici Lake a écrit :
> >
> >> I don’t remember why I wrote it this way, but today I don’t care much
> >> about INT_MIN here. I think the code has become too complex for what it
> >> meant to do. Also, maybe I should have sticked to int instead of
> trying to
> >> unsigned, but it’s too late to change that :)
> >>
> >>
> > Yes, keeping everything int would have been easier. Or even templating
> the
> > class on a (signed) integer type, so that applications expecting enormous
> > files could deal with them.
>
> That’s the kind of things I’d be happy to deal with later, but I
> first want to push 3.1 outside (smashing all the not too hard bugs
> I can see), and then support move semantics in C++ in 3.2.
>
>
> >> What do you think of my proposal restoring std::max?
> >
> > It's certainly clearer, although of course there are issues with integer
> > overflow on huge inputs (but we don’t care about those, right? -:).
>
> Let’s wait for bug reports about that :)
>
> > Since min is a parameter to add_, you can just make it int instead of
> > unsigned. No point in static_cast(min).
>
> Meh… That way I was sure that min was nonnegative, hence that the
> unsigned I return is « really » nonnegative.
>
> But you’re right, let’s make it simpler.
>
> Good to go?
>
> commit 59f931a50fcd3f3687377507e6e13a6a236d236b
> Author: Akim Demaille
> Date: Sat Aug 18 16:37:47 2018 +0200
>
> C++: fix portability issue with MSVC 2017
>
> Visual Studio issues a C4146 warning on '-static_cast(rhs)'.
> The code is weird, probably to cope with INT_MIN. Let's go back to
> using std::max (whose header is still included in position.hh...) like
> originally, but with the needed casts.
>
> Reported by 長田偉伸, and with help from Rici Lake.
>
> See also
> http://lists.gnu.org/archive/html/bug-bison/2013-02/msg0.html
> and commit 75ae8299840bbd854fa2474d38402bbb933c6511.
>
> * data/location.cc (position::add_): Take min as an int.
> Use std::max.
> While here, get rid of a couple of useless inlines.
>
> diff --git a/THANKS b/THANKS
> index 33f23ed7..c655e3c6 100644
> --- a/THANKS
> +++ b/THANKS
> @@ -173,6 +173,7 @@ Wolfram Wagnerw...@mpi-sb.mpg.de
> Wwp subscr...@free.fr
> xolodho xolo...@gmail.com
> Zack Weinberg z...@codesourcery.com
> +長田偉伸 cbh34...@iret.co.jp
>
> Many people are not named here because we lost track of them. We
> thank them! Please, help us keeping this list up to date.
> diff --git a/data/location.cc b/data/location.cc
> index 3cc949df..07f1ca62 100644
> --- a/data/location.cc
> +++ b/data/location.cc
> @@ -73,12 +73,10 @@ m4_define([b4_position_define],
> unsigned column;
>
>private:
> -/// Compute max(min, lhs+rhs) (provided min <= lhs).
> -static unsigned add_ (unsigned lhs, int rhs, unsigned min)
> +/// Compute max(min, lhs+rhs).
> +static unsigned add_ (unsigned lhs, int rhs, int min)
> {
> - return (0 < rhs || -static_cast(rhs) < lhs
> - ? rhs + lhs
> - : min);
> + return static_cast(std::max(min, static_cast(lhs) +
> rhs));
> }
>};
>
> @@ -134,7 +132,7 @@ m4_define([b4_position_define],
> ** \param pos a reference to the position to redirect
> */
>template
> - inline std::basic_ostream&
> + std::basic_ostream&
>operator<< (std::basic_ostream& ostr, const position& pos)
>{
> if (pos.filename)
> @@ -271,7 +269,7 @@ m4_define([b4_location_define],
> ** Avoid duplicate information.
> */
>template
> - inline std::basic_ostream&
> + std::basic_ostream&
>operator<< (std::basic_ostream& ostr, const location& loc)
>{
> unsigned end_col = 0 < loc.end.column ? loc.end.column - 1 : 0;
>
>
Re: position.hh compile error C4146 (VisualStudio 2017)
On Sat, Aug 18, 2018, 10:03 Akim Demaille wrote: > Hi Rici! > > > Le 18 août 2018 à 16:45, Rici Lake a écrit : > > > > Hi, Akim > > > > I'd try > > > > return 0 < rhs || 0 - (static_cast(rhs)) < lhs > > Good idea! > > > Or the redundant but harmless > > > > return 0 < rhs || static_cast(-(static_cast int>(rhs))) < lhs > > > > C4146 is a warning, not an error. It's telling you that if u is > unsigned, -u is still unsigned. The intent is help people avoid doing > things like: > > > > if ( i > -2147483648) ... > > > > which is broken (with 32-bit integers, assuming i is an int) because the > constant is an unsigned int and therefore the comparison will be unsigned, > which will have unexpected results. Or, at least, MS assumes the results > will be unexpected. > > > > As I understand it, your code is precisely trying to avoid the undefined > behaviour possible when using -rhs, if rhs happens to be equal to INT_MIN. > I don’t see any problem with it, but you have to convince Visual Studio > that you know what you're doing. > > I don’t remember why I wrote it this way, but today I don’t care much > about INT_MIN here. I think the code has become too complex for what it > meant to do. Also, maybe I should have sticked to int instead of trying to > unsigned, but it’s too late to change that :) > > > Yes, keeping everything int would have been easier. Or even templating the > class on a (signed) integer type, so that applications expecting enormous > files could deal with them. > > By the way, contrary to the claim in the comment in add_, that function > only works if min is 0 or 1. If min were 2, the return value could be 1. If > the intent is that b4_location_initial_column and b4_location_initial_line > only have 0 or 1 as possible values, it might be clearer to make them > booleans with names like b4_location_column_is_one_based and > b4_location_line_is_one_based. :-) > > Yes, absolutely. But I guess it’s too late too. > > What do you think of my proposal restoring std::max? It's certainly clearer, although of course there are issues with integer overflow on huge inputs (but we don't care about those, right? -:). Since min is a parameter to add_, you can just make it int instead of unsigned. No point in static_cast(min).
Re: position.hh compile error C4146 (VisualStudio 2017)
Hi Rici! > Le 18 août 2018 à 16:45, Rici Lake a écrit : > > Hi, Akim > > I'd try > > return 0 < rhs || 0 - (static_cast(rhs)) < lhs Good idea! > Or the redundant but harmless > > return 0 < rhs || static_cast(-(static_cast int>(rhs))) < lhs > > C4146 is a warning, not an error. It's telling you that if u is unsigned, -u > is still unsigned. The intent is help people avoid doing things like: > > if ( i > -2147483648) ... > > which is broken (with 32-bit integers, assuming i is an int) because the > constant is an unsigned int and therefore the comparison will be unsigned, > which will have unexpected results. Or, at least, MS assumes the results will > be unexpected. > > As I understand it, your code is precisely trying to avoid the undefined > behaviour possible when using -rhs, if rhs happens to be equal to INT_MIN. I > don’t see any problem with it, but you have to convince Visual Studio that > you know what you're doing. I don’t remember why I wrote it this way, but today I don’t care much about INT_MIN here. I think the code has become too complex for what it meant to do. Also, maybe I should have sticked to int instead of trying to unsigned, but it’s too late to change that :) > By the way, contrary to the claim in the comment in add_, that function only > works if min is 0 or 1. If min were 2, the return value could be 1. If the > intent is that b4_location_initial_column and b4_location_initial_line only > have 0 or 1 as possible values, it might be clearer to make them booleans > with names like b4_location_column_is_one_based and > b4_location_line_is_one_based. :-) Yes, absolutely. But I guess it’s too late too. What do you think of my proposal restoring std::max?
Re: position.hh compile error C4146 (VisualStudio 2017)
I tried it and confirmed that it works normally.
Thanks,
2018-08-18 23:45 GMT+09:00 Akim Demaille :
> Please keep bug-bison@gnu.org.
>
>> Le 18 août 2018 à 15:53, 長田偉伸 a écrit :
>>
>> it is still broken.
>>
>>> Can I install this fix?
>>
>>> Would you also consider reporting this bug to MS?
>>
>> Sorry, I can not do anything.
>>
>> Because I can understand English only a little.
>> (I am Japanese)
>
> That’s fine :)
>
> The code was actually stupid. Please try this. Thanks!
>
> commit d32b0c7ee5d39bd33589fc5e62985a68ab78087e
> Author: Akim Demaille
> Date: Sat Aug 18 16:37:47 2018 +0200
>
> C++: fix portability issue with MSVC 2017
>
> Visual Studio dies with parse error on '-static_cast(rhs)'.
> But the code
>
> return (0 < rhs || -static_cast(rhs) < lhs
> ? rhs + lhs
> : min);
>
> was really all wrong, it should have been
>
> return (0 < rhs || static_cast(-rhs) < lhs
> ? rhs + lhs
> : min);
>
> yet let's go back to using std::max (whose header is still included in
> position.hh...) like originally, but with all the needed casts.
>
> Reported by 長田偉伸.
>
> See also
> http://lists.gnu.org/archive/html/bug-bison/2013-02/msg0.html
> and commit 75ae8299840bbd854fa2474d38402bbb933c6511.
>
> * data/location.cc (position::add_): Use std::max.
>
> diff --git a/THANKS b/THANKS
> index 33f23ed7..c655e3c6 100644
> --- a/THANKS
> +++ b/THANKS
> @@ -173,6 +173,7 @@ Wolfram Wagnerw...@mpi-sb.mpg.de
> Wwp subscr...@free.fr
> xolodho xolo...@gmail.com
> Zack Weinberg z...@codesourcery.com
> +長田偉伸 cbh34...@iret.co.jp
>
> Many people are not named here because we lost track of them. We
> thank them! Please, help us keeping this list up to date.
> diff --git a/data/location.cc b/data/location.cc
> index 3cc949df..622ffd61 100644
> --- a/data/location.cc
> +++ b/data/location.cc
> @@ -73,12 +73,11 @@ m4_define([b4_position_define],
> unsigned column;
>
>private:
> -/// Compute max(min, lhs+rhs) (provided min <= lhs).
> +/// Compute max(min, lhs+rhs).
> static unsigned add_ (unsigned lhs, int rhs, unsigned min)
> {
> - return (0 < rhs || -static_cast(rhs) < lhs
> - ? rhs + lhs
> - : min);
> + return static_cast(std::max(static_cast(min),
> +static_cast(lhs) + rhs));
> }
>};
>
>
Re: position.hh compile error C4146 (VisualStudio 2017)
Hi, Akim
I'd try
return 0 < rhs || 0 - (static_cast(rhs)) < lhs
Or the redundant but harmless
return 0 < rhs || static_cast(-(static_cast(rhs))) < lhs
C4146 is a warning, not an error. It's telling you that if u is unsigned,
-u is still unsigned. The intent is help people avoid doing things like:
if ( i > -2147483648) ...
which is broken (with 32-bit integers, assuming i is an int) because the
constant is an unsigned int and therefore the comparison will be unsigned,
which will have unexpected results. Or, at least, MS assumes the results
will be unexpected.
As I understand it, your code is precisely trying to avoid the undefined
behaviour possible when using -rhs, if rhs happens to be equal to INT_MIN.
I don't see any problem with it, but you have to convince Visual Studio
that you know what you're doing.
By the way, contrary to the claim in the comment in add_, that function
only works if min is 0 or 1. If min were 2, the return value could be 1. If
the intent is that b4_location_initial_column and b4_location_initial_line
only have 0 or 1 as possible values, it might be clearer to make them
booleans with names like b4_location_column_is_one_based and
b4_location_line_is_one_based. :-)
Rici
2018-08-18 8:53 GMT-05:00 長田偉伸 :
> > Sorry, it is not clear to me: it still works, or it is still broken?
>
> it is still broken.
>
> > Can I install this fix?
>
> > Would you also consider reporting this bug to MS?
>
> Sorry, I can not do anything.
>
> Because I can understand English only a little.
> (I am Japanese)
>
>
>
> 2018-08-18 22:40 GMT+09:00 Akim Demaille :
> > Please, always keep the list in CC.
> >
> >> Le 18 août 2018 à 15:26, 長田偉伸 a écrit :
> >>
> >> Thank you for your early reply
> >>
> >> I changed the file and then compiled it.
> >> However, the situation has not changed.
> >
> > Sorry, it is not clear to me: it still works, or it is still broken?
> >
> >> - position.hh: 107 - 114
> >>/// Compute max(min, lhs+rhs) (provided min <= lhs).
> >>static unsigned add_ (unsigned lhs, int rhs, unsigned min)
> >>{
> >> //return (0 < rhs || -static_cast(rhs) < lhs
> >> return (0 < rhs || -(static_cast(rhs)) < lhs
> >> ? rhs + lhs
> >> : min);
> >>}
> >> ——
> >
> > Can I install this fix?
> >
> > Would you also consider reporting this bug to MS? Thanks!
>
>
Re: position.hh compile error C4146 (VisualStudio 2017)
Please keep bug-bison@gnu.org.
> Le 18 août 2018 à 15:53, 長田偉伸 a écrit :
>
> it is still broken.
>
>> Can I install this fix?
>
>> Would you also consider reporting this bug to MS?
>
> Sorry, I can not do anything.
>
> Because I can understand English only a little.
> (I am Japanese)
That’s fine :)
The code was actually stupid. Please try this. Thanks!
commit d32b0c7ee5d39bd33589fc5e62985a68ab78087e
Author: Akim Demaille
Date: Sat Aug 18 16:37:47 2018 +0200
C++: fix portability issue with MSVC 2017
Visual Studio dies with parse error on '-static_cast(rhs)'.
But the code
return (0 < rhs || -static_cast(rhs) < lhs
? rhs + lhs
: min);
was really all wrong, it should have been
return (0 < rhs || static_cast(-rhs) < lhs
? rhs + lhs
: min);
yet let's go back to using std::max (whose header is still included in
position.hh...) like originally, but with all the needed casts.
Reported by 長田偉伸.
See also
http://lists.gnu.org/archive/html/bug-bison/2013-02/msg0.html
and commit 75ae8299840bbd854fa2474d38402bbb933c6511.
* data/location.cc (position::add_): Use std::max.
diff --git a/THANKS b/THANKS
index 33f23ed7..c655e3c6 100644
--- a/THANKS
+++ b/THANKS
@@ -173,6 +173,7 @@ Wolfram Wagnerw...@mpi-sb.mpg.de
Wwp subscr...@free.fr
xolodho xolo...@gmail.com
Zack Weinberg z...@codesourcery.com
+長田偉伸 cbh34...@iret.co.jp
Many people are not named here because we lost track of them. We
thank them! Please, help us keeping this list up to date.
diff --git a/data/location.cc b/data/location.cc
index 3cc949df..622ffd61 100644
--- a/data/location.cc
+++ b/data/location.cc
@@ -73,12 +73,11 @@ m4_define([b4_position_define],
unsigned column;
private:
-/// Compute max(min, lhs+rhs) (provided min <= lhs).
+/// Compute max(min, lhs+rhs).
static unsigned add_ (unsigned lhs, int rhs, unsigned min)
{
- return (0 < rhs || -static_cast(rhs) < lhs
- ? rhs + lhs
- : min);
+ return static_cast(std::max(static_cast(min),
+static_cast(lhs) + rhs));
}
};
Re: position.hh compile error C4146 (VisualStudio 2017)
> Sorry, it is not clear to me: it still works, or it is still broken?
it is still broken.
> Can I install this fix?
> Would you also consider reporting this bug to MS?
Sorry, I can not do anything.
Because I can understand English only a little.
(I am Japanese)
2018-08-18 22:40 GMT+09:00 Akim Demaille :
> Please, always keep the list in CC.
>
>> Le 18 août 2018 à 15:26, 長田偉伸 a écrit :
>>
>> Thank you for your early reply
>>
>> I changed the file and then compiled it.
>> However, the situation has not changed.
>
> Sorry, it is not clear to me: it still works, or it is still broken?
>
>> - position.hh: 107 - 114
>>/// Compute max(min, lhs+rhs) (provided min <= lhs).
>>static unsigned add_ (unsigned lhs, int rhs, unsigned min)
>>{
>> //return (0 < rhs || -static_cast(rhs) < lhs
>> return (0 < rhs || -(static_cast(rhs)) < lhs
>> ? rhs + lhs
>> : min);
>>}
>> ——
>
> Can I install this fix?
>
> Would you also consider reporting this bug to MS? Thanks!
Re: position.hh compile error C4146 (VisualStudio 2017)
Hi!
> Le 18 août 2018 à 07:36, 長田偉伸 a écrit :
>
> [Description]
>
>I used flex to generate the source code.
>After that, when I compiled using VisualStudio 2017, an error of
> C4146 occurred.
>
>I modified the file containing the error as follows.
>
># (original) position.hh:111
> return (0 < rhs || -static_cast(rhs) < lhs
>
># (edited) position.hh:111
> return (0 < rhs || -1 * static_cast(rhs) < lhs
>
>I compiled it again and confirmed that no error occurred
>
>
> [Environment]
>OS: Windows10
>Visual Studio 2017 (C++)
>
>Version: bison (GNU Bison) 3.0.5
>
>
> Thank you,
Amazing…
According to http://www.externsoft.ch/media/swf/cpp11-iso.html,
we have in the grammar of C++ (with focus on what matters here):
unary_expression:
• postfix_expression
• unary_operator cast_expression
cast_expression
• unary_expression
• '(' type_id ')' cast_expression
unary_operator:
• '+'
• '-'
postfix_expression:
• 'dynamic_cast' '<' type_id '>' '(' expression ')'
• ‘static_cast' '<' type_id '>' '(' expression ')'
So it is my reading that Visual Studio is wrong here.
I don’t like the -1 *, could you please rather check that parens would suffice?
return (0 < rhs || -(static_cast(rhs)) < lhs
Thanks!
