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.

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

Const POD Default Constructor #2300

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Oct 10, 2017

Fixes clang(-tidy) errors of the kind [clang-diagnostic-error]:
"default initialization of an object of const type '...' without a user-provided default constructor":

First approach before discussion:

const typename Difference::template GetDifference<0> Dx;
// to
const typename Difference::template GetDifference<0> Dx{};

Final approach: add explicit constructors to POD classes that are used as const:

    struct GetDifference
    {
        static constexpr uint32_t direction = T_direction;

        HDINLINE GetDifference()
        {
        }
        // ...
    };

@ax3l ax3l added component: core in PIConGPU (core application) warning code produces/produced a warning labels Oct 10, 2017
@ax3l ax3l added this to the 0.4.0 / 1.0.0: Next Stable milestone Oct 10, 2017
@ax3l ax3l requested a review from psychocoderHPC October 10, 2017 19:02
@psychocoderHPC
Copy link
Member

psychocoderHPC commented Oct 11, 2017

Based on this stackoverflow post we should add a explicit default constructor to GetDifference, the reult of traits::FieldPositionand ShiftMeIfYouCan.
The reason is that a type without a constructor is a POD data type and can be uninitialized. An uninitialized const variable make no sense. With the current PR we defining from the outside that we a fine with that uninitialized type but this requires internal knowledge about the implementation.

We should add a default constructor e.g. GetDifference() = default;

@ax3l ax3l changed the title Fix Clang Warnings Fix Clang Warnings: Default Constructor Oct 11, 2017
@ax3l
Copy link
Member Author

ax3l commented Oct 27, 2017

thanks!

I thinks it's actually exactly the opposite implementation wise, instead of explicitly not adding a constructor in the current class with = default; we just add one with GetDifference(){} instead. (As the SO answer references, this is weird for a class with only static constexpr members but currently part of the C++ standard...)

the problem seems to be, that POD classes do not get a default constructor in C++ and a const POD without constructor makes no sense as it will always stay uninitialized (also, the D in POD is thought to store data...)

@ax3l ax3l changed the title Fix Clang Warnings: Default Constructor Fix Clang Warnings: const POD Default Constructor Oct 27, 2017
@ax3l ax3l changed the title Fix Clang Warnings: const POD Default Constructor Const POD Default Constructor Oct 27, 2017
@ax3l ax3l force-pushed the fix-defaultConstructor branch from 1fa7414 to 1102b4e Compare October 27, 2017 08:39
Fixes clang(-tidy) errors of the kind [clang-diagnostic-error]:
"default initialization of an object of const type '...'
without a user-provided default constructor"

Adds default constructors to POC classes that are used as `const`.
@ax3l ax3l force-pushed the fix-defaultConstructor branch from 1102b4e to 2b6fd81 Compare October 27, 2017 08:40
@@ -112,6 +119,10 @@ namespace traits
typedef VectorVector3D3V type;
};

HDINLINE FieldPosition()
Copy link
Member

Choose a reason for hiding this comment

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

In c++11 it should be FieldPosition = default; Please to not add a prefix qualifier else we will get nvcc or clang warnings.

Copy link
Member

Choose a reason for hiding this comment

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

Ohh missread your comment. = default is equal to the old c++98 way.
Whe can check it in a mini ap with std::is_pod.

Copy link
Member

Choose a reason for hiding this comment

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

offline discussed: = default is not equal to a hand written empty constructor :-(

@ax3l
Copy link
Member Author

ax3l commented Oct 27, 2017 via email

@psychocoderHPC psychocoderHPC merged commit 4826d6a into ComputationalRadiationPhysics:dev Nov 1, 2017
@ax3l ax3l deleted the fix-defaultConstructor branch November 1, 2017 09:25
@ax3l
Copy link
Member Author

ax3l commented Aug 8, 2018

Observed today: this is a clang 3.8 weirdness and gone in 3.9+:

Has problems to compile with the error message above:

#include <string>
#include <iostream>

constexpr float f = 4.5;

struct A
{
    static constexpr float value = f;

    float
    operator()() const
    {
        return f;
    }
};

int main()
{
    A const a;
    std::cout << a() << std::endl;

    return 0;
}

We should just require clang 3.9+ (also supported in CUDA 8.0+)

@sbastrakov
Copy link
Member

sbastrakov commented Aug 8, 2018

Yuck! Even a trivial example with const object + automatic default constructor does not work in clang 3.8.

@ax3l
Copy link
Member Author

ax3l commented Aug 8, 2018

Yep, also tried it online: https://godbolt.org/g/sfhPXz

@sbastrakov
Copy link
Member

Omg, it was actually a standard-conforming behaviour, which then was marked as a defect: http://open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#253

@ax3l
Copy link
Member Author

ax3l commented Aug 8, 2018

omg -.-

@ax3l ax3l mentioned this pull request Aug 8, 2018
psychocoderHPC pushed a commit to psychocoderHPC/picongpu that referenced this pull request Aug 7, 2024
106a4975f4 fix getFunctionAttributes for the SYCL backend
f36e1156af update CUDA version in CI
3f8456973e use inline for CUDA/HIP code when rdc is on, otherwise use static
8b9cc3c557 fix gh-pages jobA
89d5ce671c Ignore VI temporary files
4b7bd17493 Fix the device used by KernelExecutionFixture (ComputationalRadiationPhysics#2344)
2c386dc5e9 Make alpaka follow CMAKE_CUDA_RUNTIME_LIBRARY
2d652dd233 Add thread count to CPU blocks accelerators (ComputationalRadiationPhysics#2338)
dbc5ebe1e9 Fix complex math pow test (ComputationalRadiationPhysics#2336)
4995c5b22a Fix isValidWorkDivKernel to use the correct device
f571ce9197 Remove unnecessary include
a26cdbcd41 Re-enable the KernelNoTemplateGpu test
a9217fb780 Link libcudart even when libcurand is not used
9c8614143b Suppress GCC warning about casting a function to void*
ba169cdc52 Rewrite the getValidWorkDivForKernel tests
948eb757d4 Fix getValidWorkDivForKernel tests for the SYCL CPU backend
f6f94f13b5 Fix getValidWorkDivForKernel tests for the CUDA backend
f612f971a0 Reduce code duplications in matrixMulWithMdSpan (ComputationalRadiationPhysics#2326)
d1cc2e01c1 Add a matrix multiplication example using mdspan
536a183cce Add missing whitespace in enqueue log messages
81d4410eec Reduce code duplication in CUDA/HIP kernel launch
6fdec14904  add remove-restrict
5323600508 CI: improve script utils
01d123e605 fix missing C++20 STL for ICPX in the CI
d254bcd6a3 ctest: display only output of tests, which failed
c9b8c941af change documentation
b9ed742913 remove getValidWorkDiv itself
048ef8afca use getValidWorkDivForKernel in kernelfixture of tests
38805498f0 fix random strategies
4f175420f2 remove getValidWorkDiv first
7f08120428 CI_FILTER: ^linux_nvcc11.*
789344f019 ALPAKA_FN_HOST is not a type
4efdb9dc63 fix explicit instantiation issue
fe4106f88a CI_FILTER: ^linux_nvcc11.*gcc9
e6b4881b70 CI_FILTER: ^linux_nvcc11.*gcc9
e3e760ed9e make conv2dmdspan use kernelbundle
62efffe605 Add getValidWorkDivForKernel function and KernelBundle with tests
690da679bd Let the SYCL queue implement `ConceptCurrentThreadWaitFor`, `ConceptGetDev` and `ConceptQueue` (ComputationalRadiationPhysics#2314)
995c57b54b set alpaka_CXX_STANDARD in the job generator
6ad09baa38 remove nvcc11.0 and nvcc11.1 support (ComputationalRadiationPhysics#2310)
0775f7c066 clang-format and fix typo
18eeeb7b49 move complex declaration to internal namespace
3468d2f8ac add trait IsKernelTriviallyCopyable
3015eae06b update CI container to version 3.2
56c0e416bc Update Catch2 to v3.5.2 (ComputationalRadiationPhysics#2300)

git-subtree-dir: thirdParty/alpaka
git-subtree-split: 106a4975f48dc38cc34f6a2494a3d16774282951
psychocoderHPC pushed a commit to psychocoderHPC/picongpu that referenced this pull request Aug 9, 2024
ab3092357b Use shared CUDA libraries by default
106a4975f4 fix getFunctionAttributes for the SYCL backend
f36e1156af update CUDA version in CI
3f8456973e use inline for CUDA/HIP code when rdc is on, otherwise use static
8b9cc3c557 fix gh-pages jobA
89d5ce671c Ignore VI temporary files
4b7bd17493 Fix the device used by KernelExecutionFixture (ComputationalRadiationPhysics#2344)
2c386dc5e9 Make alpaka follow CMAKE_CUDA_RUNTIME_LIBRARY
2d652dd233 Add thread count to CPU blocks accelerators (ComputationalRadiationPhysics#2338)
dbc5ebe1e9 Fix complex math pow test (ComputationalRadiationPhysics#2336)
4995c5b22a Fix isValidWorkDivKernel to use the correct device
f571ce9197 Remove unnecessary include
a26cdbcd41 Re-enable the KernelNoTemplateGpu test
a9217fb780 Link libcudart even when libcurand is not used
9c8614143b Suppress GCC warning about casting a function to void*
ba169cdc52 Rewrite the getValidWorkDivForKernel tests
948eb757d4 Fix getValidWorkDivForKernel tests for the SYCL CPU backend
f6f94f13b5 Fix getValidWorkDivForKernel tests for the CUDA backend
f612f971a0 Reduce code duplications in matrixMulWithMdSpan (ComputationalRadiationPhysics#2326)
d1cc2e01c1 Add a matrix multiplication example using mdspan
536a183cce Add missing whitespace in enqueue log messages
81d4410eec Reduce code duplication in CUDA/HIP kernel launch
6fdec14904  add remove-restrict
5323600508 CI: improve script utils
01d123e605 fix missing C++20 STL for ICPX in the CI
d254bcd6a3 ctest: display only output of tests, which failed
c9b8c941af change documentation
b9ed742913 remove getValidWorkDiv itself
048ef8afca use getValidWorkDivForKernel in kernelfixture of tests
38805498f0 fix random strategies
4f175420f2 remove getValidWorkDiv first
7f08120428 CI_FILTER: ^linux_nvcc11.*
789344f019 ALPAKA_FN_HOST is not a type
4efdb9dc63 fix explicit instantiation issue
fe4106f88a CI_FILTER: ^linux_nvcc11.*gcc9
e6b4881b70 CI_FILTER: ^linux_nvcc11.*gcc9
e3e760ed9e make conv2dmdspan use kernelbundle
62efffe605 Add getValidWorkDivForKernel function and KernelBundle with tests
690da679bd Let the SYCL queue implement `ConceptCurrentThreadWaitFor`, `ConceptGetDev` and `ConceptQueue` (ComputationalRadiationPhysics#2314)
995c57b54b set alpaka_CXX_STANDARD in the job generator
6ad09baa38 remove nvcc11.0 and nvcc11.1 support (ComputationalRadiationPhysics#2310)
0775f7c066 clang-format and fix typo
18eeeb7b49 move complex declaration to internal namespace
3468d2f8ac add trait IsKernelTriviallyCopyable
3015eae06b update CI container to version 3.2
56c0e416bc Update Catch2 to v3.5.2 (ComputationalRadiationPhysics#2300)

git-subtree-dir: thirdParty/alpaka
git-subtree-split: ab3092357bb0b917b4cc396ce49e47f7ac1924e1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: core in PIConGPU (core application) warning code produces/produced a warning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants