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

[refactor](exceptionsafe) disallow call new method explicitly #18830

Merged
merged 12 commits into from
Apr 21, 2023

Conversation

yiguolei
Copy link
Contributor

@yiguolei yiguolei commented Apr 19, 2023

Proposed changes

  1. disallow call new method explicitly
  2. force to use create_shared or create_unique to use shared ptr
  3. placement new is allowed

reference https://abseil.io/tips/42 to add factory method to all class.
I think we should follow this guide because if throw exception in new method, the program will terminate.

Problem summary

Describe your changes.

Checklist(Required)

  • Does it affect the original behavior
  • Has unit tests been added
  • Has document been added or modified
  • Does it need to update dependencies
  • Is this PR support rollback (If NO, please explain WHY)

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@yiguolei
Copy link
Contributor Author

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// Not use template like cowhelper to implements this feature because it has problem
// during inherits
// TODO try to allow make_unique
#define DISALLOW_EXPILICT_NEW(TypeName) \
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro is not used [clang-diagnostic-unused-macros]

#define DISALLOW_EXPILICT_NEW(TypeName)                                                     \
        ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// Not use template like cowhelper to implements this feature because it has problem
// during inherits
// TODO try to allow make_unique
#define ENABLE_FACTORY_CREATOR(TypeName) \
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro is not used [clang-diagnostic-unused-macros]

#define ENABLE_FACTORY_CREATOR(TypeName)                                                    \
        ^

@@ -53,10 +54,11 @@ namespace vectorized {
*/
class MutableBlock;
class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: friend declaration of 'make_unique' does not match any declaration in namespace 'std' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block);
    ^

be/src/common/factory_creator.h:52: expanded from macro 'ENABLE_FACTORY_CREATOR'

    friend std::unique_ptr<T> std::make_unique(Args&&... args);\
                                   ^

@@ -53,10 +54,11 @@ namespace vectorized {
*/
class MutableBlock;
class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: extra ';' inside a class [clang-diagnostic-extra-semi]

Suggested change
ENABLE_FACTORY_CREATOR(Block)
ENABLE_FACTORY_CREATOR(Block)

@@ -383,6 +385,8 @@ using BlocksPtr = std::shared_ptr<Blocks>;
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: friend declaration of 'make_unique' does not match any declaration in namespace 'std' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock);
    ^

be/src/common/factory_creator.h:52: expanded from macro 'ENABLE_FACTORY_CREATOR'

    friend std::unique_ptr<T> std::make_unique(Args&&... args);\
                                   ^

@@ -383,6 +385,8 @@ using BlocksPtr = std::shared_ptr<Blocks>;
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: extra ';' inside a class [clang-diagnostic-extra-semi]

Suggested change
ENABLE_FACTORY_CREATOR(MutableBlock)
ENABLE_FACTORY_CREATOR(MutableBlock)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -53,10 +54,11 @@ namespace vectorized {
*/
class MutableBlock;
class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: friend declaration of 'make_unique' does not match any declaration in namespace 'std' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block)
    ^

be/src/common/factory_creator.h:52: expanded from macro 'ENABLE_FACTORY_CREATOR'

    friend std::unique_ptr<T> std::make_unique(Args&&... args);\
                                   ^

@@ -383,6 +385,8 @@
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: friend declaration of 'make_unique' does not match any declaration in namespace 'std' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock)
    ^

be/src/common/factory_creator.h:52: expanded from macro 'ENABLE_FACTORY_CREATOR'

    friend std::unique_ptr<T> std::make_unique(Args&&... args);\
                                   ^

@yiguolei
Copy link
Contributor Author

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

auto block = std::make_unique<vectorized::Block>(_output_tuple_desc->slots(),
real_block_size,
true /*ignore invalid slots*/);
auto block = vectorized::Block::create_unique(_output_tuple_desc->slots(),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member access into incomplete type 'const doris::TupleDescriptor' [clang-diagnostic-error]

                auto block = vectorized::Block::create_unique(_output_tuple_desc->slots(),
                                                                                ^

be/src/vec/core/block.h:51: forward declaration of 'doris::TupleDescriptor'

class TupleDescriptor;
      ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// Not use template like cowhelper to implements this feature because it has problem
// during inherits
// TODO try to allow make_unique
#define ENABLE_FACTORY_CREATOR(TypeName) \
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro is not used [clang-diagnostic-unused-macros]

#define ENABLE_FACTORY_CREATOR(TypeName)                                                       \
        ^

@@ -67,10 +68,11 @@ namespace vectorized {
class MutableBlock;

class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: expected ';' after return statement [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block)
    ^

be/src/common/factory_creator.h:45: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, void* place) { return ::operator delete(ptr, place) }      \
                                                                                        ^

@@ -67,10 +68,11 @@
class MutableBlock;

class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block)
    ^

be/src/common/factory_creator.h:43: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, std::size_t size) { return ::operator delete(ptr, size); } \
                                                               ^

/usr/include/c++/11/new:179: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete  (void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:143: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete(void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:152: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete(void*, std::align_val_t)
     ^

/usr/include/c++/11/new:129: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete(void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:154: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete(void*, std::align_val_t, const std::nothrow_t&)
     ^

@@ -67,10 +68,11 @@
class MutableBlock;

class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use of undeclared identifier 'size' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block)
    ^

be/src/common/factory_creator.h:44: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete[](void* ptr, std::size_t sz) { return ::operator delete(ptr, size); } \
                                                                                      ^

@@ -397,6 +399,8 @@
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: expected ';' after return statement [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock)
    ^

be/src/common/factory_creator.h:45: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, void* place) { return ::operator delete(ptr, place) }      \
                                                                                        ^

@@ -397,6 +399,8 @@
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock)
    ^

be/src/common/factory_creator.h:43: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, std::size_t size) { return ::operator delete(ptr, size); } \
                                                               ^

/usr/include/c++/11/new:179: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete  (void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:143: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete(void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:152: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete(void*, std::align_val_t)
     ^

/usr/include/c++/11/new:129: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete(void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:154: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete(void*, std::align_val_t, const std::nothrow_t&)
     ^

@@ -397,6 +399,8 @@
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use of undeclared identifier 'size' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock)
    ^

be/src/common/factory_creator.h:44: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete[](void* ptr, std::size_t sz) { return ::operator delete(ptr, size); } \
                                                                                      ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// Not use template like cowhelper to implements this feature because it has problem
// during inherits
// TODO try to allow make_unique
#define ENABLE_FACTORY_CREATOR(TypeName) \
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro is not used [clang-diagnostic-unused-macros]

#define ENABLE_FACTORY_CREATOR(TypeName)                                                         \
        ^

@@ -67,10 +68,11 @@ namespace vectorized {
class MutableBlock;

class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: expected ';' after return statement [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block)
    ^

be/src/common/factory_creator.h:45: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, void* place) { return ::operator delete(ptr, place) }        \
                                                                                        ^

@@ -67,10 +68,11 @@
class MutableBlock;

class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block)
    ^

be/src/common/factory_creator.h:43: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, std::size_t size) { return ::operator delete(ptr, size); }   \
                                                               ^

/usr/include/c++/11/new:179: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete  (void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:143: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete(void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:152: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete(void*, std::align_val_t)
     ^

/usr/include/c++/11/new:129: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete(void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:154: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete(void*, std::align_val_t, const std::nothrow_t&)
     ^

@@ -397,6 +399,8 @@
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: expected ';' after return statement [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock)
    ^

be/src/common/factory_creator.h:45: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, void* place) { return ::operator delete(ptr, place) }        \
                                                                                        ^

@@ -397,6 +399,8 @@
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock)
    ^

be/src/common/factory_creator.h:43: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, std::size_t size) { return ::operator delete(ptr, size); }   \
                                                               ^

/usr/include/c++/11/new:179: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete  (void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:143: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete(void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:152: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete(void*, std::align_val_t)
     ^

/usr/include/c++/11/new:129: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete(void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:154: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete(void*, std::align_val_t, const std::nothrow_t&)
     ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// Not use template like cowhelper to implements this feature because it has problem
// during inherits
// TODO try to allow make_unique
#define ENABLE_FACTORY_CREATOR(TypeName) \
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro is not used [clang-diagnostic-unused-macros]

#define ENABLE_FACTORY_CREATOR(TypeName)                                                           \
        ^

@@ -67,10 +68,11 @@ namespace vectorized {
class MutableBlock;

class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block)
    ^

be/src/common/factory_creator.h:43: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, std::size_t size) { return ::operator delete(ptr, size); }     \
                                                               ^

/usr/include/c++/11/new:179: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete  (void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:143: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete(void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:152: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete(void*, std::align_val_t)
     ^

/usr/include/c++/11/new:129: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete(void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:154: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete(void*, std::align_val_t, const std::nothrow_t&)
     ^

@@ -67,10 +68,11 @@
class MutableBlock;

class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete[]' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block)
    ^

be/src/common/factory_creator.h:44: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete[](void* ptr, std::size_t size) { return ::operator delete[](ptr, size); } \
                                                                 ^

/usr/include/c++/11/new:180: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete[](void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:145: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete[](void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:160: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete[](void*, std::align_val_t)
     ^

/usr/include/c++/11/new:131: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete[](void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:162: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete[](void*, std::align_val_t, const std::nothrow_t&)
     ^

@@ -397,6 +399,8 @@
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock)
    ^

be/src/common/factory_creator.h:43: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, std::size_t size) { return ::operator delete(ptr, size); }     \
                                                               ^

/usr/include/c++/11/new:179: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete  (void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:143: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete(void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:152: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete(void*, std::align_val_t)
     ^

/usr/include/c++/11/new:129: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete(void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:154: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete(void*, std::align_val_t, const std::nothrow_t&)
     ^

@@ -397,6 +399,8 @@
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete[]' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock)
    ^

be/src/common/factory_creator.h:44: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete[](void* ptr, std::size_t size) { return ::operator delete[](ptr, size); } \
                                                                 ^

/usr/include/c++/11/new:180: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete[](void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:145: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete[](void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:160: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete[](void*, std::align_val_t)
     ^

/usr/include/c++/11/new:131: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete[](void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:162: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete[](void*, std::align_val_t, const std::nothrow_t&)
     ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// Not use template like cowhelper to implements this feature because it has problem
// during inherits
// TODO try to allow make_unique
//
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro is not used [clang-diagnostic-unused-macros]

#define ENABLE_FACTORY_CREATOR(TypeName)                                                    \
        ^

@@ -67,10 +68,11 @@ namespace vectorized {
class MutableBlock;

class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block)
    ^

be/src/common/factory_creator.h:43: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, std::size_t size) { ::operator delete(ptr, size); }     \
                                                        ^

/usr/include/c++/11/new:179: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete  (void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:143: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete(void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:152: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete(void*, std::align_val_t)
     ^

/usr/include/c++/11/new:129: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete(void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:154: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete(void*, std::align_val_t, const std::nothrow_t&)
     ^

@@ -67,10 +68,11 @@
class MutableBlock;

class Block {
ENABLE_FACTORY_CREATOR(Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete[]' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(Block)
    ^

be/src/common/factory_creator.h:44: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete[](void* ptr, std::size_t size) { ::operator delete[](ptr, size); } \
                                                          ^

/usr/include/c++/11/new:180: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete[](void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:145: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete[](void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:160: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete[](void*, std::align_val_t)
     ^

/usr/include/c++/11/new:131: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete[](void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:162: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete[](void*, std::align_val_t, const std::nothrow_t&)
     ^

@@ -397,6 +399,8 @@
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock)
    ^

be/src/common/factory_creator.h:43: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete(void* ptr, std::size_t size) { ::operator delete(ptr, size); }     \
                                                        ^

/usr/include/c++/11/new:179: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete  (void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:143: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete(void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:152: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete(void*, std::align_val_t)
     ^

/usr/include/c++/11/new:129: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete(void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:154: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete(void*, std::align_val_t, const std::nothrow_t&)
     ^

@@ -397,6 +399,8 @@
using BlocksPtrs = std::shared_ptr<std::vector<BlocksPtr>>;

class MutableBlock {
ENABLE_FACTORY_CREATOR(MutableBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no matching function for call to 'operator delete[]' [clang-diagnostic-error]

    ENABLE_FACTORY_CREATOR(MutableBlock)
    ^

be/src/common/factory_creator.h:44: expanded from macro 'ENABLE_FACTORY_CREATOR'

    void operator delete[](void* ptr, std::size_t size) { ::operator delete[](ptr, size); } \
                                                          ^

/usr/include/c++/11/new:180: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'void *' for 2nd argument; take the address of the argument with &

inline void operator delete[](void*, void*) _GLIBCXX_USE_NOEXCEPT { }
            ^

/usr/include/c++/11/new:145: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'const std::nothrow_t' for 2nd argument

void operator delete[](void*, const std::nothrow_t&) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:160: candidate function not viable: no known conversion from 'std::size_t' (aka 'unsigned long') to 'std::align_val_t' for 2nd argument

void operator delete[](void*, std::align_val_t)
     ^

/usr/include/c++/11/new:131: candidate function not viable: requires 1 argument, but 2 were provided

void operator delete[](void*) _GLIBCXX_USE_NOEXCEPT
     ^

/usr/include/c++/11/new:162: candidate function not viable: requires 3 arguments, but 2 were provided

void operator delete[](void*, std::align_val_t, const std::nothrow_t&)
     ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// during inherits
// TODO try to allow make_unique
//
#define ENABLE_FACTORY_CREATOR(TypeName) \
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro is not used [clang-diagnostic-unused-macros]

#define ENABLE_FACTORY_CREATOR(TypeName)                                                    \
        ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// during inherits
// TODO try to allow make_unique
//
#define ENABLE_FACTORY_CREATOR(TypeName) \
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro is not used [clang-diagnostic-unused-macros]

#define ENABLE_FACTORY_CREATOR(TypeName)                                                     \
        ^

@yiguolei
Copy link
Contributor Author

run buildall

1 similar comment
@yiguolei
Copy link
Contributor Author

run buildall

@yiguolei
Copy link
Contributor Author

run p0

@yiguolei
Copy link
Contributor Author

run buildall

Copy link
Contributor

@Gabriel39 Gabriel39 left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Apr 21, 2023
@github-actions
Copy link
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@yiguolei yiguolei merged commit 63a76ed into apache:master Apr 21, 2023
gnehil pushed a commit to gnehil/doris that referenced this pull request Apr 21, 2023
…#18830)

disallow call new method explicitly
force to use create_shared or create_unique to use shared ptr
placement new is allowed
reference https://abseil.io/tips/42 to add factory method to all class.
I think we should follow this guide because if throw exception in new method, the program will terminate.

---------

Co-authored-by: yiguolei <yiguolei@gmail.com>
xiaokang pushed a commit to xiaokang/doris that referenced this pull request Apr 22, 2023
…#18830)

disallow call new method explicitly
force to use create_shared or create_unique to use shared ptr
placement new is allowed
reference https://abseil.io/tips/42 to add factory method to all class.
I think we should follow this guide because if throw exception in new method, the program will terminate.

---------

Co-authored-by: yiguolei <yiguolei@gmail.com>
xiaokang added a commit to xiaokang/doris that referenced this pull request Apr 24, 2023
Reminiscent pushed a commit to Reminiscent/doris that referenced this pull request May 15, 2023
…#18830)

disallow call new method explicitly
force to use create_shared or create_unique to use shared ptr
placement new is allowed
reference https://abseil.io/tips/42 to add factory method to all class.
I think we should follow this guide because if throw exception in new method, the program will terminate.

---------

Co-authored-by: yiguolei <yiguolei@gmail.com>
gnehil pushed a commit to gnehil/doris that referenced this pull request Jul 13, 2023
gnehil pushed a commit to gnehil/doris that referenced this pull request Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by one committer. area/vectorization reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants