Hi, Nikita,
Despite the commit has in the title, comments below apply to four
commits:
$ git log --oneline 7d5165894282^..0f6fd23e1604
0f6fd23e160 cleanup: add new(thd) to Sql_alloc and Item
e0afb5f08b5 cleanup: further replace thd->alloc() with operator new
8daea2e14b2 cleanup: add operator new(size_t, const THD*) for single-object allocations
7d516589428 cleanup: add operator new[](size_t, const THD*)
On Dec 06, Nikita Malyavin wrote:
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> index aff02df69ec..299c8298606 100644
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -1279,11 +1279,5 @@ class Query_arena
> { return state == STMT_CONVENTIONAL_EXECUTION; }
>
> template <typename T=char>
> - inline T* alloc(size_t size) const
> - {
> - return (T*)alloc_root(mem_root, sizeof(T)*size);
> - }
> -
> - template <typename T=char>
> inline T* calloc(size_t size) const
Hmm, you didn't remove thd->calloc<>()
I understand why, but that kind of makes the API inconsistent,
perhaps it would've been cleaner to keep thd->alloc<>()
and only implement new (thd) and new (arena) for Sql_alloc?
May be not only for Sql_alloc. There are
1. pod types. they can be alloc'ed an calloc'ed, I think
thd->alloc/thd->calloc is fine for them
2. non-pod types, can be allocated on the heap and on a mem_root.
your new (thd)/delete (thd) operators are best for them.
3. non-pod types that can only be allocated on a mem_root. Sql_alloc can
be used for them, if it'll disallow non-placement new.
I feel like we don't need to make a distinction between 2 and 3, in that
case Sql_alloc can simply go away.
> {
> diff --git a/plugin/feedback/feedback.cc b/plugin/feedback/feedback.cc
> index bc8a98c11ae..dcc2e4417e5 100644
> --- a/plugin/feedback/feedback.cc
> +++ b/plugin/feedback/feedback.cc
> @@ -103,22 +103,22 @@ static COND* make_cond(THD *thd, TABLE_LIST *tables, LEX_STRING *filter)
> nrc->resolve_in_table_list_only(tables);
> nrc->select_lex= tables->select_lex;
>
> - res= new (thd) Item_cond_or(thd);
> + res= new (thd) Item_cond_or(thd);
> if (!res)
> return OOM;
>
> for (; filter->str; filter++)
> {
> - Item_field *fld= new (thd) Item_field(thd, nrc, db, table,
> + Item_field *fld= new (thd) Item_field(thd, nrc, db, table,
> field);
> - Item_string *pattern= new (thd) Item_string(thd, filter->str,
> + Item_string *pattern= new (thd) Item_string(thd, filter->str,
> (uint) filter->length, cs);
you might want to go through the diff and fix the indentation
> - Item_string *escape= new (thd) Item_string(thd, "\\", 1, cs);
> + Item_string *escape= new (thd) Item_string(thd, "\\", 1, cs);
>
> if (!fld || !pattern || !escape)
> return OOM;
>
> - Item_func_like *like= new (thd) Item_func_like(thd, fld, pattern,
> + Item_func_like *like= new (thd) Item_func_like(thd, fld, pattern,
> escape, 0);
>
> if (!like)
> diff --git a/sql/item.h b/sql/item.h
> index 631fd1b19f8..b0cf03c1435 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -857,8 +857,9 @@ class Item :public Value_source,
> static void *operator new(size_t size);
>
> public:
> - static void *operator new(size_t size, MEM_ROOT *mem_root) throw ()
> + static void *operator new(size_t size, MEM_ROOT *mem_root) noexcept
> { return alloc_root(mem_root, size); }
> + static void *operator new(size_t size, const THD *thd) noexcept;
> static void operator delete(void *ptr,size_t size) { TRASH_FREE(ptr, size); }
> static void operator delete(void *ptr, MEM_ROOT *mem_root) {}
Better inherit it from Sql_alloc
> diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
> index d86fdf5c5d6..e0adc5c3bb4 100644
> --- a/sql/item_cmpfunc.cc
> +++ b/sql/item_cmpfunc.cc
> @@ -2007,7 +2007,7 @@ bool Item_func_interval::fix_length_and_dec(THD *thd)
>
> if (not_null_consts)
> {
> - intervals= thd->alloc<interval_range>(rows - 1);
> + intervals= new(current_thd) interval_range[rows - 1];
current_thd ?
> if (!intervals)
> return true;
>
> diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc
> index 008384a6590..21364286260 100644
> --- a/sql/item_subselect.cc
> +++ b/sql/item_subselect.cc
> @@ -6086,8 +6086,8 @@ bool Ordered_key::init(int col_idx)
>
> // TIMOUR: check for mem allocation err, revert to scan
>
> - key_columns= thd->alloc<Item_field*>(1);
> - compare_pred= thd->alloc<Item_func_lt*>(1);
> + key_columns= new(thd) Item_field*();;
> + compare_pred= new(thd) Item_func_lt*();;
;;
>
> key_columns[0]= new (thd) Item_field(thd, tbl->field[col_idx]);
> /* Create the predicate (tmp_column[i] < outer_ref[i]). */
> diff --git a/sql/opt_subselect.h b/sql/opt_subselect.h
> index 8b4dd1f0092..37902c6eebe 100644
> --- a/sql/opt_subselect.h
> +++ b/sql/opt_subselect.h
> @@ -357,7 +357,7 @@ uint get_number_of_tables_at_top_level(JOIN *join);
> the tuple.
> */
>
> -class SJ_TMP_TABLE : public Sql_alloc
> +class SJ_TMP_TABLE
why?
> {
> public:
> /*
> diff --git a/sql/sql_alloc.h b/sql/sql_alloc.h
> index f5d2d4e8b1a..e643a9f817d 100644
> --- a/sql/sql_alloc.h
> +++ b/sql/sql_alloc.h
> @@ -23,17 +23,21 @@
> class Sql_alloc
> {
> public:
> - static void *operator new(size_t size) throw ()
> + static void *operator new(size_t size) noexcept
> {
> return thd_alloc(_current_thd(), size);
> }
can this be disallowed completely?
> - static void *operator new[](size_t size) throw ()
> + static void *operator new[](size_t size) noexcept
> {
> return thd_alloc(_current_thd(), size);
> }
> - static void *operator new[](size_t size, MEM_ROOT *mem_root) throw ()
> +
> + static void *operator new[](size_t size, const THD *thd) noexcept;
> + static void *operator new(size_t size, const THD *thd) noexcept;
> +
> + static void *operator new[](size_t size, MEM_ROOT *mem_root) noexcept
> { return alloc_root(mem_root, size); }
> - static void *operator new(size_t size, MEM_ROOT *mem_root) throw()
> + static void *operator new(size_t size, MEM_ROOT *mem_root) noexcept
> { return alloc_root(mem_root, size); }
> static void operator delete(void *ptr, size_t size) { TRASH_FREE(ptr, size); }
> static void operator delete(void *, MEM_ROOT *){}
> diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> index 7847389dabc..37869da63ae 100644
> --- a/sql/sql_class.cc
> +++ b/sql/sql_class.cc
> @@ -8831,3 +8831,46 @@ LEX_CSTRING make_string(THD *thd, const char *start_ptr,
> size_t length= end_ptr - start_ptr;
> return {strmake_root(thd->mem_root, start_ptr, length), length};
> }
> +void* operator new(size_t size, const Query_arena *thd) noexcept
> +{
> + return alloc_root(thd->mem_root, size);
> +}
> +
> +void operator delete[](void *ptr, const Query_arena *thd) noexcept
> +{}
> +
> +void operator delete(void *ptr, const Query_arena *thd) noexcept
> +{}
if you put them into sql_class.h - will they be inlined?
Regards,
Sergei
Chief Architect, MariaDB Server
and security(a)mariadb.org
