[Bug c++/105470] New: ranged for loop whitespace parsing

[Bug c++/105470] New: ranged for loop whitespace parsing

[roland at logikalsolutions dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

            Bug ID: 105470
           Summary: ranged for loop whitespace parsing
           Product: gcc
           Version: 11.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: roland at logikalsolutions dot com
  Target Milestone: ---

I have this library 

https://sourceforge.net/u/roland_hughes/csscintilla/ci/default/tree/

On Ubuntu 20.04 it builds clean.

roland@roland-HP-EliteDesk-800-G2-SFF:/$ gcc --version
gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


On Ubuntu 22.04 it does not.

roland@roland-u22-04-VirtualBox:~/sf_projects/roland_hughes-csscintilla/copperspice$
gcc --version
gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


Fresh build directory established
[37/39] Building CXX object
src/CMakeFiles/CsScintilla.dir/__/CsScintillaEditBase/CsScintillaEditBase.cpp.o
FAILED:
src/CMakeFiles/CsScintilla.dir/__/CsScintillaEditBase/CsScintillaEditBase.cpp.o 
/usr/bin/c++ -DBUILD_DATE=\"05/03/2022\" -DCsScintilla_EXPORTS
-I/home/roland/sf_projects/csscintilla_build/src
-I/home/roland/sf_projects/roland_hughes-csscintilla/copperspice/src
-I/home/roland/sf_projects/roland_hughes-csscintilla/copperspice/../include
-I/home/roland/sf_projects/roland_hughes-csscintilla/copperspice/../src
-I/home/roland/sf_projects/roland_hughes-csscintilla/copperspice/CsScintillaEditBase
-I/home/roland/sf_projects/roland_hughes-csscintilla/copperspice/CsScintillaEdit
-isystem /usr/lib/cs_lib/include -isystem /usr/lib/cs_lib/include/QtCore
-isystem /usr/lib/cs_lib/include/QtGui -O3 -DNDEBUG -fPIC -DSCINTILLA_CS
-DMAKING_LIBRARY=1 -DSCI_LEXER=1 -D_CRT_SECURE_NO_DEPRECATE=1 -DNDEBUG=1 -Wall
-Wextra -Wuninitialized -pedantic -Werror -std=gnu++17 -MD -MT
src/CMakeFiles/CsScintilla.dir/__/CsScintillaEditBase/CsScintillaEditBase.cpp.o
-MF
src/CMakeFiles/CsScintilla.dir/__/CsScintillaEditBase/CsScintillaEditBase.cpp.o.d
-o
src/CMakeFiles/CsScintilla.dir/__/CsScintillaEditBase/CsScintillaEditBase.cpp.o
-c
/home/roland/sf_projects/roland_hughes-csscintilla/copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp
/home/roland/sf_projects/roland_hughes-csscintilla/copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp:
In member function QString CsScintillaEditBase::keyMapText(int):
/home/roland/sf_projects/roland_hughes-csscintilla/copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp:1034:62:
error: loop variable it of type const
std::pair<Scintilla::Internal::KeyModifiers, Scintilla::Message>& binds to a
temporary constructed from type const std::pair<const
Scintilla::Internal::KeyModifiers, Scintilla::Message>
[-Werror=range-loop-construct]
 1034 |     for ( const std::pair<KeyModifiers, Scintilla::Message> &it :
sqt->kmap.GetKeyMap() )
      |                                                              ^~
/home/roland/sf_projects/roland_hughes-csscintilla/copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp:1034:62:
note: use non-reference type const
std::pair<Scintilla::Internal::KeyModifiers, Scintilla::Message> to make the
copy explicit or const std::pair<const Scintilla::Internal::KeyModifiers,
Scintilla::Message>& to prevent copying
cc1plus: all warnings being treated as errors
ninja: build stopped: subcommand failed.


If you look at the error message, this version of the compiler demands the & be
against > instead of out with the variable.

This appears to be a whitespace parsing bug.

[Bug c++/105470] ranged for loop whitespace parsing

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mpolacek at gcc dot gnu.org

--- Comment #1 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Can you please provide a stand-alone test case?

[Bug c++/105470] ranged for loop whitespace parsing

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-05-03
             Status|UNCONFIRMED                 |WAITING
     Ever confirmed|0                           |1

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Add -save-temps and attach the .ii file that is produced.

[Bug c++/105470] ranged for loop whitespace parsing

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
It looks more like you're using -Werror and a new warning in the new GCC is
being turned into an error, because you asked for it.

[Bug c++/105470] ranged for loop whitespace parsing

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |INVALID

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Roland Hughes from comment #0)
> If you look at the error message, this version of the compiler demands the &
> be against > instead of out with the variable.

No it doesn't.

> This appears to be a whitespace parsing bug.

I don't think so.

The warning is completely correct, and the code should be fixed.

for ( const std::pair<KeyModifiers, Scintilla::Message> &it : someMap )

This iterates over a map, with values of type:

std::pair<const KeyModifiers, Scintilla::Message>

But the loop is using a reference to a different type:

std::pair<KeyModifiers, Scintilla::Message>

That causes a temporary to be created on every iteration of the loop, and the
const-refernece binds to the temporary. The new GCC is smart enough to tell you
this, and you've asked for that warning to be a fatal error.

Either don't enable errors for that warning, or fix the code to avoid the
unnecessary temporaries being created.

[Bug c++/105470] ranged for loop whitespace parsing

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
GCC even tells you exactly how to fix the code.

Here's the GCC error reformatted to make it a bit easier to read:

error: loop variable it of type
const std::pair<Scintilla::Internal::KeyModifiers, Scintilla::Message>&
binds to a temporary constructed from type
const std::pair<const Scintilla::Internal::KeyModifiers, Scintilla::Message>
[-Werror=range-loop-construct]

note: use non-reference type
const std::pair<Scintilla::Internal::KeyModifiers, Scintilla::Message>
to make the copy explicit or
const std::pair<const Scintilla::Internal::KeyModifiers, Scintilla::Message>&
to prevent copying

cc1plus: all warnings being treated as errors

[Bug c++/105470] ranged for loop whitespace parsing

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #5)
> GCC even tells you exactly how to fix the code.
> 
> Here's the GCC error reformatted to make it a bit easier to read:
> 
> error: loop variable it of type
> const std::pair<Scintilla::Internal::KeyModifiers, Scintilla::Message>&
> binds to a temporary constructed from type
> const std::pair<const Scintilla::Internal::KeyModifiers, Scintilla::Message>
> [-Werror=range-loop-construct]
> 
> note: use non-reference type
> const std::pair<Scintilla::Internal::KeyModifiers, Scintilla::Message>
> to make the copy explicit or
> const std::pair<const Scintilla::Internal::KeyModifiers, Scintilla::Message>&
> to prevent copying
> 
> cc1plus: all warnings being treated as errors

auto comes in handy here so you could also just do:
for ( const auto &it : sqt->kmap.GetKeyMap() )

And not worry about the type matches exactly.

[Bug c++/105470] ranged for loop whitespace parsing

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, or use TheMapType::const_iterator. If you're going to name the type
explicitly, you need to be sure to use the correct type.

[Bug c++/105470] ranged for loop whitespace parsing

[roland at logikalsolutions dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

--- Comment #8 from Roland Hughes <roland at logikalsolutions dot com> ---
(In reply to Marek Polacek from comment #1)
> Can you please provide a stand-alone test case?

I will work on this over the weekend.

[Bug c++/105470] ranged for loop whitespace parsing

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

--- Comment #9 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to Roland Hughes from comment #8)
> (In reply to Marek Polacek from comment #1)
> > Can you please provide a stand-alone test case?
> 
> I will work on this over the weekend.

Looks like that's not necessary anymore.  Jon already explained why.

[Bug c++/105470] ranged for loop whitespace parsing

[roland at logikalsolutions dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

--- Comment #10 from Roland Hughes <roland at logikalsolutions dot com> ---
(In reply to Marek Polacek from comment #1)
> 
> The warning is completely correct, and the code should be fixed.
> 
> for ( const std::pair<KeyModifiers, Scintilla::Message> &it : someMap )
> 
> This iterates over a map, with values of type:
> 
> std::pair<const KeyModifiers, Scintilla::Message>
> 

No, it doesn't.

[code]
roland@roland-HP-EliteDesk-800-G2-SFF:~/sf_projects/roland_hughes-csscintilla$
grep -irn GetKeyMap *
copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp:1031:    // the fact
GetKeyMap() returns const means we can't use an iterator
copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp:1034:    for ( const
std::pair<KeyModifiers, Scintilla::Message> &it : sqt->kmap.GetKeyMap() )
copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp:1059:    auto
findResult = std::find_if( std::begin( sqt->kmap.GetKeyMap() ),
copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp:1060:                  
                 std::end( sqt->kmap.GetKeyMap() ),
copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp:1066:    if (
findResult != std::end( sqt->kmap.GetKeyMap() ) )
copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp:4314:const
std::map<Scintilla::Internal::KeyModifiers, Scintilla::Message>
&CsScintillaEditBase::GetKeyMap() const noexcept
copperspice/CsScintillaEditBase/CsScintillaEditBase.cpp:4316:    return
sqt->kmap.GetKeyMap();
copperspice/CsScintillaEditBase/CsScintillaEditBase.h:79:    const
std::map<Scintilla::Internal::KeyModifiers, Scintilla::Message> &GetKeyMap()
const noexcept;
src/KeyMap.h:60:        const std::map<KeyModifiers, Scintilla::Message>
&GetKeyMap() const noexcept;
src/KeyMap.cxx:50:const std::map<KeyModifiers, Message> &KeyMap::GetKeyMap()
const noexcept {
[/code]


There is no definition of that map anywhere in the codebase where KeyModifiers
is declared const.

The method being called returns a const & to the entire map, but there is no
const declared within the map.

[Bug c++/105470] ranged for loop whitespace parsing

[roland at logikalsolutions dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

--- Comment #11 from Roland Hughes <roland at logikalsolutions dot com> ---
(In reply to Marek Polacek from comment #9)
> (In reply to Roland Hughes from comment #8)
> > (In reply to Marek Polacek from comment #1)
> > > Can you please provide a stand-alone test case?
> > 
> > I will work on this over the weekend.
> 
> Looks like that's not necessary anymore.  Jon already explained why.

It is actually necessary, see my other comment.

There is no declaration anywhere in the code base where the map contains a
constant value.

This is not resolved.

[Bug c++/105470] ranged for loop whitespace parsing

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105470

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Roland Hughes from comment #10)
> There is no definition of that map anywhere in the codebase where
> KeyModifiers is declared const.

That's just how std::map works.

std::map<A, B>::value_type is std::pair<const A, B>.
See the definition of value_type at
https://en.cppreference.com/w/cpp/container/map

So when you iterate over a map<A, B> the values you get back are pair<const A,
B>.

> The method being called returns a const & to the entire map, but there is no
> const declared within the map.

There is in the map's value_type though, and that's what you iterate over with
the range-based for loop.