Add more compilation flags #258
Merged
Conversation
This file contains 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
|
@lamphamsy It would be great if you could review the changes in the commit "fix Doxygen documentation (flagged by Most of the errors flagged by
If you have suggestions for those, I would be more than happy to update this commit with better descriptions/documentation 🙂 |
slaperche-scality
force-pushed
the
eh/add_compiler_warnings
branch
4 times, most recently
from
2348531 to
4dfa7b2
Compare
Type qualifiers (such as const) are ignored on function return type.
Add missing noreturn attribute.
`narrow_cast` is a simple wrapper around `static_cast` and should be used when you cast to narrow a value. That way, if one day we have a bug related to narrowing, we can simply grep for the specific `narrow_cast` instead of the more general `static_cast`.
Don't use C-like cast, C++ offers a wider range of cast, each own with its own semantic and rules.
Note the most of the occurences are fixed by replacing some pure virtual methods by virtual methods with a default body that does nothing (as most of the children classes were doing nothing…). Also remove a truly unused parameter from `unpack`.
Either add missing prototypes or mark the function with static.
Here is a quick drill down, for those unfamiliar with this aspect of
C++.
What's is this about?
=====================
The first thing to note is that there is no overloading across scopes,
and derived class scopes are not an exception.
Concretely, that means that this code:
struct A {
int foo(int i) { std::cout << "foo(int): "; return i+1; }
};
struct B : public A {
double f(double d) { std::cout << "foo(double): "; return d+1.3; }
};
[…]
B* pd = new B;
std::cout << pb->f(2) << '\n';
Will produce:
f(double): 3.3
Because the best method is only looked up into D scope, so even though a
better match is available for `int` in B, it's not found and the version
for `double` is used.
The compiler warning is triggered when the hidden function is virtual,
as shown in the example from the man:
struct A {
virtual void f();
};
struct B: public A {
void f(int);
};
the A class version of f is hidden in B, and code like:
B* b;
b->f();
fails to compile.
What about us?
==============
Our codebase is affected by this problem, the hidden functions are:
- void FecCode<T>::decode_apply(const DecodeContext<T>&, Buffers<T>&, Buffers<T>&)
- void FecCode<T>::decode_prepare(const DecodeContext<T>&, const std::vector<Properties>&, off_t, Buffers<T>&)
- void FecCode<T>::encode_post_process(Buffers<T>&, std::vector<Properties>&, off_t)
- void FecCode<T>::encode(Buffers<T>&, std::vector<quadiron::Properties>&, off_t, Buffers<T>&)
- void FourierTransform<T>::fft_inv(Buffers<T>&, Buffers<T>&)
- void FourierTransform<T>::fft(Buffers<T>&, Buffers<T>&)
- void FourierTransform<T>::ifft(Buffers<T>&, Buffers<T>&)
- void RingModN<T>::neg(Buffers<T>&) const
- void RingModN<T>::neg(size_t, T*) const
You can see that most of them are `Buffers` version of pure abtract
methods that works on `Vector`.
Since the `Vector` versions are pure abstract, children classes must
override them and by doing so mask the overloaded versions that work on
`Buffers` (except if they also override the `Buffers` versions`, but
that's not always the case).
Example:
- `fft::Radix2` override both `Vector` and `Buffers` versions
=> no compiler warning.
- `fft::CooleyTukey` doesn't override the `Buffers` versions
=> we have a warning.
But our code compile/works!
===========================
Yeah, by happenstance.
Because most of these `Buffers` version are defined as no-op by default
and if a child class doesn't override it that propably means that it's
never called and thus yeah, our code compile.
But that doesn't change the fact that the code is brittle, and we should
clean these up!
How do we fix this?
===================
By explicitely asking for the methods lookup to consider the scope of
the parent class, which is done with an using-declaration.
Appendix: why this weird and confusing behavior‽
================================================
Because if we were looking up into the parents' scopes, then the code
have certain unforeseen and potentially dangerous behavior.
Consider the following code:
class Base {
int x;
public:
virtual void copy(Base* p) { x = p-> x; }
};
class Derived{
int xx;
public:
virtual void copy(Derived* p) { xx = p->xx; Base::copy(p); }
};
void f(Base a, Derived b)
{
a.copy(&b); // ok: copy Base part of b
b.copy(&a); // BUG: Base::copy is called and `xx` is not copied!
}
If we were looking up into the parent's scope, then we would find
`Base::copy(Base*)` which is a better match for the second call to copy
BUT it would lead to object slicing, which is unexpected here!
References
==========
- http://www.stroustrup.com/bs_faq2.html#overloadderived
- https://stackoverflow.com/a/1629074
VLA are not supported in C++, that's a C99 feature not a C++ one (and even in C, it becames optional since C11). Even if they were supported, it's not a good idea to use them (especially with an unbounded size such as `this->n`) this is a recipe for easy stack overflows. Now, dumbly replacing VLAs by `std::vector` simply won't do (the code becomes 50% slower). But, we can implement a scratch space inside the `gf::NF4` object that can replaces the VLAs and get a nice 10% speedup :)
`typename` is preferred in this context, and this is what we use in the rest of the codebase.
slaperche-scality
force-pushed
the
eh/add_compiler_warnings
branch
from
3982145 to
82386e3
Compare
Since we have a destructor, we add:
- a copy constructor
- a move constructor
- a copy assignment operator
- a move assignment operator
Clang was complaining about it (`-Wdeprecated`), and rightly so!
definition of implicit copy constructor for 'Vector<unsigned int>'
is deprecated because it has a user-declared destructor
This will definitely help us spot dubious code and maintain a good level of code quality. Next step would be to enable more lint in clang-tidy.
The debug mode is the one with the more warnings, so this is the one that should be tested (if it compiles in Debug, it will compile in Release), but since we use conditionnal compilation for SIMD we should make sure that every codepath is clean. Compilation in all SIMD mode in release is done by the test job anyway.
We should use list append, not string concat to add the SIMD flag.
`k` is a member variable and is ambiguous with local variable (such as in `taylor_expand_t2`). A more global renaming is planned later, when we will add `-Wshadow` to our compilation flags.
slaperche-scality
force-pushed
the
eh/add_compiler_warnings
branch
from
82386e3 to
b9cf4c0
Compare
lamphamsy
approved these changes
Oct 31, 2018
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.
In order to keep a good level of code quality, this PR introduces makes Clang and GCC stricter.
The set of flags are the ones that were used on the DCSS project.
The next step will be to enable more lint check in clang-tidy, but that PR is already big enough as it is.
Also, before moving onto the linter, there are still two more warnings that I want to enable:
-Wconversionand-Wshadow.Those will require a litlte bit more rework so they will be done in a dedicated PR:
-Wconversion: it's probably wise to tackle Indices should be declared assize_t#243 first-Wshadowwill require a bit of wide renaming (but in the end will improve the code readability) and I'll probably rework the visibility of some fields on the way (too many public fields in some of our classes right now).