Compiler warnings

[ellert]

Fix -Wmisleading-indentation warnings
Fix -Wpessimizing-move warnings

[abh3]

I really do want to accept this commit. But really, brain-dead compilers that don't recognize that a "return" is the end of an execution sequence? That used to be the case with "break" statement but that got corrected. Maybe this is a compiler version problem (i.e. old compiler) Let's see what Michal thinks of this.

[ellert]

No - it is quite a new compiler.

$ gcc --version
gcc (GCC) 9.3.1 20200408 (Red Hat 9.3.1-2)
Copyright © 2019 Free Software Foundation, Inc.

[ellert]

The warning is about the confusing indentation, not about the compiler not recognising "the end of an execution sequence". If you are unhappy about the "else" I added to resolve the warning, the alternative way to resolve the warning is to simply remove the spaces at the beginning of the line:

   if (isTLS) return linkXQ.TLS_Peek(Buff, Blen, timeout);
   return linkXQ.Peek(Buff, Blen, timeout);

This will also resolve the warning. Since I assumed that using the confusing indentation was done in order to have the two return statements appear above eachother I opted for adding the "else" since this choice allowed to keep this. If you prefer the solution without the "else" I can remove the spaces instead.

[simonmichal]

Hi guys,

The warnings shouldn't be a problem as for newer compilers we are adding following flags:

• -Wno-error=misleading-indentation
• -Wno-error=pessimizing-move

That said I do agree it would be nicer if we could drop the flags and fix the warnings.

Regarding the misleading indentation warning: IMHO the indentations in question are a bit unconventional ;-) but it's your call @abh3 ;-)

Regarding the pessimizing move warning: the reason why I haven't fixed this myself is that although new compilers are obliged to do copy elision on return, I'm not sure if the old once give this guaranty (slc6, slc7). Let me double check ;-)

[ellert]

The problem is that with the std::move the compiler can not do copy elision and you end up with less efficient code than without the std::move.
https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c/

[xrootd-dev]

If there are just a couple of moves to fix we could do if-def based on compiler version. As for indentation, is it just that one place where the complaint occurs? If so, then there is no reason not just fix it and not set the no complain option. If it's all over the code, I'd just keep the option on until we have time to lint the code. One person's misleading indentation is another person's beautiful code. So, I'm not willing to spend time on this at the moment.

…

On Thu, 28 May 2020, simonmichal wrote: Hi guys, The warnings shouldn't be a problem as for newer compilers we are adding following flags: - `-Wno-error=misleading-indentation` - `-Wno-error=pessimizing-move` That said I do agree it would be nicer if we could drop the flags and fix the warnings. Regarding the misleading indentation warning: IMHO the indentations in question are a bit unconventional ;-) but it's your call @abh3 ;-) Regarding the pessimizing move warning: the reason why I haven't fixed this myself is that although new compilers are obliged to do copy elision on return, I'm not sure if the old once give this guaranty (slc6, slc7). Let me double check ;-) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #1209 (comment) ######################################################################## Use REPLY-ALL to reply to list To unsubscribe from the XROOTD-DEV list, click the following link: https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1

[simonmichal]

@ellert : correct but this applies only to new compilers, actually from what I see copy elision is guaranteed only in some case and only starting with c++17, before that there's no guarantee:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0135r0.html

This means that if we optimise our code for copy elision we will end up with more efficient code on latest fedora platforms but probably less efficient on old platforms like slc6/cc7. Therefore I would vote for leaving the -Wno-error=pessimizing-move option.

Also, even with the newest compilers the overhead of std::move on return is negligible IMHO.

[ellert]

Doing some test:

$ cat test.cpp
#include <iostream>

class A {
public:
  A();
  A(const A&);
  A(const A&&);
};

A::A() {
  std::cout << "A::A()" << std::endl;
}

A::A(const A&) {
  std::cout << "A::A(const A&)" << std::endl;
}

A::A(const A&&) {
  std::cout << "A::A(const A&&)" << std::endl;
}

A A1() {
  A a;
  return a;
}

A A2() {
  A a;
  return std::move(a);
}

int main() {
  A a1(A1());
  A a2(A2());
}

On Fedora 32 (gcc 10):

$ g++ --version
g++ (GCC) 10.1.1 20200507 (Red Hat 10.1.1-1)

$ g++ -std=c++11 -o test test.cpp
$ ./test
A::A()
A::A()
A::A(const A&&)
$ g++ -std=c++14 -o test test.cpp
$ ./test
A::A()
A::A()
A::A(const A&&)
$ g++ -std=c++17 -o test test.cpp
$ ./test
A::A()
A::A()
A::A(const A&&)
$ g++ -std=c++20 -o test test.cpp
$ ./test
A::A()
A::A()
A::A(const A&&)

On CentOS 8 (gcc 8):

$ g++ --version
g++ (GCC) 8.3.1 20191121 (Red Hat 8.3.1-5)

$ g++ -std=c++11 -o test test.cpp
$ ./test 
A::A()
A::A()
A::A(const A&&)
$ g++ -std=c++14 -o test test.cpp
$ ./test 
A::A()
A::A()
A::A(const A&&)
$ g++ -std=c++17 -o test test.cpp
$ ./test 
A::A()
A::A()
A::A(const A&&)
$ g++ -std=c++20 -o test test.cpp
g++: error: unrecognized command line option ‘-std=c++20’; did you mean ‘-std=c++2a’?
$ g++ -std=c++2a -o test test.cpp
$ ./test 
A::A()
A::A()
A::A(const A&&)

On Centos 7 (gcc 4.8)

$ g++ --version
g++ (GCC) 4.8.5 20150623 (Red Hat 4.8.5-39)

$ g++ -std=c++11 -o test test.cpp 
$ ./test
A::A()
A::A()
A::A(const A&&)
$ g++ -std=c++14 -o test test.cpp
g++: error: unrecognized command line option '-std=c++14'
$ g++ -std=c++1y -o test test.cpp
$ ./test
A::A()
A::A()
A::A(const A&&)

On CentOS 6 (gcc 4.4):

$ g++ --version
g++ (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23)

$ g++ -std=c++11 -o test test.cpp
cc1plus: error: unrecognized command line option "-std=c++11"
$ g++ -std=c++0x -o test test.cpp
$ ./test 
A::A()
A::A()
A::A(const A&&)

The result is the same in all cases - the return std::move is pessimising and results in a extra call to the move constructor in all cases, including CentOS 6 and 7.

[simonmichal]

@ellert : you rest your case, I'm OK with merging your PR, I'll just double check with Andy.