Fix: Miscellaneous Clang warnings #781
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pniedzielski
force-pushed
the
published/fix/warnings/misc
branch
2 times, most recently
from
c13277a to
f62fee6
Compare
hallfox
reviewed
Jun 13, 2025
hallfox
reviewed
Jun 13, 2025
| @@ -191,7 +191,7 @@ inline FileManagerImpl::FileHandler<ITER>::FileHandler( | |||
| } | |||
|
|
|||
| template <typename ITER> | |||
| inline FileManagerImpl::FileHandler<ITER>::~FileHandler() | |||
| inline FileManagerImpl::FileHandler<ITER>::FileHandler::~FileHandler() | |||
There was a problem hiding this comment.
I think this syntax doesn't compile on C++03, I believe I've been burned by this before.
There was a problem hiding this comment.
I'll give this a shot in C++03 then; if it doesn't compile, we'll have to turn that warning off.
I see in the git log that we also tried:
inline FileManagerImpl::FileHandler<ITER>::~FileHandler<ITER>()
which also doesn't compile in C++03.
There was a problem hiding this comment.
We shouldn't need this to compile in C++03: the storage tool is written in C++11, and won't compile in C++03. But our check for this only makes sure that we're not building on Solaris: building all in C++03 on Linux will fail with some C++11-isms in the storage tool. I'll fix the check to make sure CMAKE_CXX_STANDARD is C++11 or better.
But either way, clang doesn't complain about this line in C++03.
There was a problem hiding this comment.
Can we solve this issue completely by placing the nested class declaration to the outer scope?
pniedzielski
force-pushed
the
published/fix/warnings/misc
branch
from
f62fee6 to
3e4fea1
Compare
![bmq-oss-ci[bot]](https://avatars.githubusercontent.com/u/1416818?s=60&v=4){kind=small}
hallfox
approved these changes
Jun 26, 2025
This patch turns off the following warning emitted by Clang in `bmqp`:
src/groups/bmq/bmqp/bmqp_messageproperties.t.cpp:1213:23: warning: explicitly assigning value of variable of type 'bmqp::MessageProperties' to itself [-Wself-assign-overloaded]
1213 | obj2 = obj2;
| ~~~~ ^ ~~~~
1 warning generated.
This self-assignment is intentional; the test is checking that
self-assignment is properly implemented.
Signed-off-by: Patrick M. Niedzielski <[email protected]>
Now that we use Doxygen to build docs, anything that looks like HTML tags may
be interpreted that way, rather than as literals. Clang warns about this, as
follows:
src/groups/mqb/mqbblp/mqbblp_rootqueueengine.t.cpp:582:10: warning: HTML start tag prematurely ended, expected attribute name or '>' [-Wdocumentation]
582 | /// `<s_1>,<s_2>,...,<s_N>`
| ^
This patch changes some unintentional tags to use a different syntax.
Signed-off-by: Patrick M. Niedzielski <[email protected]>
Signed-off-by: Patrick M. Niedzielski <[email protected]>
Clang emits warnings of the following form in two of our tests:
src/applications/bmqstoragetool/m_bmqstoragetool_printer.t.cpp:380:6: warning: no previous prototype for function 'test4_humanReadableOffsetsTest' [-Wmissing-prototypes]
380 | void test4_humanReadableOffsetsTest()
| ^
src/applications/bmqstoragetool/m_bmqstoragetool_printer.t.cpp:380:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
380 | void test4_humanReadableOffsetsTest()
| ^
| static
This patch fixes this warning in two ways:
1. For affected tests cases, we make the test functions have `static`
linkage, as recommended by the warning and for symmetry with the
rest of the test cases in the file and the codebase.
2. For the affected printing functions, including `PrintTo` functions used by
GMock, we make the functions `inline`, as it seems they were intended to
be inlined.
Both solutions fix this warning.
Signed-off-by: Patrick M. Niedzielski <[email protected]>
Clang emits the following warning several times, once per inclusion of the
`m_bmqstoragetool_filemanager.h` file:
src/applications/bmqstoragetool/m_bmqstoragetool_filemanager.h:194:42: warning: ISO C++ requires the name after '::~' to be found in the same scope as the name before '::~' [-Wdtor-name]
194 | inline FileManagerImpl::FileHandler<ITER>::~FileHandler()
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
| ::FileHandler
This patch applies the suggested fix.
Signed-off-by: Patrick M. Niedzielski <[email protected]>
Throughout bmqstoragetool, Clang emits warnings for classes that have
virtual methods, but no virtual methods declared outside the class
definition:
src/applications/bmqstoragetool/m_bmqstoragetool_printer.h:46:7: warning: 'Printer' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Wweak-vtables]
46 | class Printer {
| ^
This patch fixes each instance of this warning by pulling the inline
method definitions to outside the class definition.
Signed-off-by: Patrick M. Niedzielski <[email protected]>
This patch moves three globally exported typedefs into the class they are used by. This avoids a warning from Clang about these typedefs shadowing similarly named typedefs in files that include this header. Signed-off-by: Patrick M. Niedzielski <[email protected]>
This patch fixes warnings from Clang due to shadowed names, using three
different strategies:
1. **Reuse the shadowed variable.** For the case of a shadowed return code
variable, it was safe to reuse the existing return code variable,
overwriting it with a different return code.
2. **Rename the shadowing variable.** It is always safe to rename the
variable that is causing the shadowing to a more descriptive name.
3. **Follow coding conventions for member variable names.** One warning
complained that a parameter to the constructor was shadowing the member
variable it was being used to initialize. The member variable should have
been prefixed with `d_`, following our coding standards.
4. **Removing the shadowed variable.** For the case of a shadowed storage key
variable, it looks like this variable is completely unused, as it is
shadowed only a few lines later, without any intermediate references. We
can safely remove the original variable.
Signed-off-by: Patrick M. Niedzielski <[email protected]>
Signed-off-by: Patrick M. Niedzielski <[email protected]>
pniedzielski
force-pushed
the
published/fix/warnings/misc
branch
from
3a0dbcc to
3fd7c7f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes miscellaneous warnings that Clang emits, which are not covered by PRs #778, #779, #780, or #681.
Please see commit messages for more details.