Don't self-initialize schema comments

[benjaminwinger]

I was getting tons of segfaults on my computer when running tests. It turned out that the new comment field in the TableSchema is being initialized to itself in one of the constructors.

Is the comment supposed to be passed as an argument to this constructor? I just default-initialized it instead, which fixed the crashes.

[Riolku]

Yikes. Sorry about that, any reason why our tests don't catch this?

Is the comment supposed to be passed as an argument to this constructor?

Not this one. By default the comment is supposed to be empty, but the second constructor is used as a "more complete" one, for copying, AFAICT.

[benjaminwinger]

It was only happening in release mode, I had to compile with the RelWithDebInfo configuration to debug it (which turned a dozen or so crashes into every single database test crashing immediately). So it might vary with the compiler: I think CI is using gcc 11, while I'm building with gcc 13.

A quick test in godbolt shows that it produces a segfault on gcc 12+, and succeeds on gcc <11. It also works fine in clang, which is the only one to produce a warning.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (4514dd4) 90.14% compared to head (d60da3b) 90.14%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2038      +/-   ##
==========================================
- Coverage   90.14%   90.14%   -0.01%     
==========================================
  Files         934      938       +4     
  Lines       33365    33364       -1     
==========================================
- Hits        30077    30076       -1     
  Misses       3288     3288              

Files Changed	Coverage Δ
src/include/catalog/table_schema.h	100.00% <100.00%> (ø)

... and 43 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Riolku]

Yet another reason to use clang 😉

[benjaminwinger]

Except there are so many warnings in the codebase that you wouldn't notice with clang normally. If we could clean up all the warnings and turn them into errors, then it could have been caught by the clang CI.

[Riolku]

ANTLR4 is particularly loud. I might look into reducing our warnings, ive always liked Werror