Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix warnings from Clang compiler #1997

Merged
merged 7 commits into from Apr 17, 2023
Merged

Fix warnings from Clang compiler #1997

merged 7 commits into from Apr 17, 2023

Conversation

amadio
Copy link
Member

@amadio amadio commented Apr 13, 2023

No description provided.

@amadio amadio requested a review from abh3 April 13, 2023 13:46
@amadio amadio self-assigned this Apr 13, 2023
@amadio
Fix Clang -Winconsistent-missing-override warnings
c074b8c 
Example warning:

In file included from .../XrdXrootdAioTask.cc:42:
src/XrdXrootd/XrdXrootdAioBuff.hh:51:25:
  warning: 'Recycle' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
virtual void            Recycle();
                        ^
src/XrdSfs/XrdSfsAio.hh:79:14: note: overridden virtual function is here
virtual void Recycle() = 0;
@amadio
Fix Clang -Wbraced-scalar-init warning
7e8486f 
 src/XrdXrootd/XrdXrootdProtocol.cc:1504:25:
   warning: braces around scalar initializer [-Wbraced-scalar-init]
    linkAioReq         = {0};
@amadio
[XrdPosix] Fix Clang -Wtautological-constant-out-of-range-compare war…
64ac102 
…ning

src/XrdPosix/XrdPosixAdmin.cc:71:10: warning: result of comparison of constant
32940614417338485 with expression of type 'unsigned int' is always false
[-Wtautological-constant-out-of-range-compare]

   if (i > std::numeric_limits<ptrdiff_t>::max() / sizeof(XrdCl::URL))
       ~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Here std::numeric_limits<ptrdiff_t>::max() / sizeof(XrdCl::URL) == 32940614417338485,
which is bigger than the largest unsigned int, therefore the
comparison will always be false. We need i to be able to hold large
enough values. Fixes 0dc292f.
@amadio
[XrdSecgsi] Fix Clang -Wfortify-source warning
242ca56 
src/XrdSecgsi/XrdSecgsiGMAPFunDN.cc:207:40: warning: 'sscanf' may overflow;
destination buffer in argument 3 has size 4096, but the corresponding specifier
may require size 4097 [-Wfortify-source]
         if (sscanf(l, "%4096s %256s", val, usr) >= 2) {
                                       ^
@amadio
[Tests] Fix typo in FileCopy test
c20746d 
Caught by a warning in Clang:

tests/XrdClTests/FileCopyTest.cc:400:62: warning: variable 'st'
is uninitialized when used within its own initialization [-Wuninitialized]
    CPPUNIT_ASSERT_XRDST( status.status == XrdCl::stError && st.code == XrdCl::errNotFound );
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/XrdClTests/../common/CppUnitXrdHelpers.hh:36:28:
 note: expanded from macro 'CPPUNIT_ASSERT_XRDST'

  XrdCl::XRootDStatus st = x;
@amadio
[Tests] Change name of macro local variable to avoid clashes
8184523 
The name st is used extensively throughout the code, so
using the same name here can cause shadowing problems or
hide typos like the one fixed in the previous commit.
@amadio
[XrdPosix] Fix build with Clang and _FORTIFY_SOURCE enabled
c165c6a 
Some of the checks added by defining _FORTIFY_SOURCE break the
build with clang with errors like the one shown below. See the
manual page for feature_test_macros(7) for more information.

xrootd/src/XrdPosix/XrdPosixPreload32.cc:375:9:
  error: redefinition of a 'extern inline' function 'pread' is not supported in C++
ssize_t pread(int fildes, void *buf, size_t nbyte, off_t offset)
        ^
/usr/include/bits/unistd.h:72:1: note: previous definition is here
pread (int __fd, void *__buf, size_t __nbytes, __off_t __offset)
^

Since distributions enable _FORTIFY_SOURCE by default, it's better
to disable it for the XrdPosix plugin when using clang. Builds with
GCC are not affected by this problem.

Fixes xrootd#1975.
abh3
abh3 reviewed Apr 14, 2023
View reviewed changes
Copy link
Member

@abh3 abh3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I do know Clang wants override but why only this set of methods. We haven't used override (though we should) in most other classes simply because that code predates "override". Also one other question about an oddity.

src/XrdPosix/XrdPosixAdmin.cc Show resolved Hide resolved
@amadio
Copy link
Member Author

amadio commented Apr 17, 2023

OK, I do know Clang wants override but why only this set of methods. We haven't used override (though we should) in most other classes simply because that code predates "override". Also one other question about an oddity.

GCC doesn't warn about it, but adding override informs the compiler about what we want to do and if the overriden/overriding method's signature is wrong or if the method we are overriding is not virtual, it will fail to compile. The commit adds the override specifier in the few places where it was expected but was not there yet. The errors shown in this example in cppreference show well the kind of checks we get by adding the override specifier.

@amadio amadio requested a review from abh3 April 17, 2023 13:32
@abh3 abh3 merged commit 0f9aa1e into xrootd:master Apr 17, 2023
14 checks passed
@amadio amadio deleted the fix-clang-warnings branch April 17, 2023 20:22
@amadio amadio added this to the 5.6 milestone May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abh3 abh3 abh3 approved these changes

Assignees

@amadio amadio

Labels
None yet
Projects
None yet
Milestone
5.6.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@amadio @abh3