Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/21331 )
Change subject: IMPALA-12950: Improve error message in case of out-of-range numeric conversions ...................................................................... Patch Set 3: Code-Review+1 (1 comment) Thank you, Daniel! http://gerrit.cloudera.org:8080/#/c/21331/2/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/21331/2/be/src/exprs/cast-functions-ir.cc@76 PS2, Line 76: } else { > It's a good point. These are the only types we'd like to cover here, so ori I think we have a bunch of mediocre options to choose from: 1. Use a default constant that will be printed when we pass an unknown type: makes the whole type enforcing a bit weaker as its main goal is to provide a name just for the defined types. 2. Using the static_assert trick with SFINAE yields ill-formed code 3. Using throw clause: ill-formed code 4. Using DCHECK: needs to duplicate the type names, and I'm not sure that the internals of DCHECK are feasible for constexpr functions +1: consteval from C++20 :) In my opinion, the least concerning is the third one, what you added in the third patchset, in the future, in case of an upgrade to C++20 it could be replaced with a consteval function. Any further inputs from other reviewers are welcome, as it's a tough choice. -- To view, visit http://gerrit.cloudera.org:8080/21331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieeed52e25f155818c35c11a8a6821708476ffb32 Gerrit-Change-Number: 21331 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Comment-Date: Tue, 23 Apr 2024 14:55:11 +0000 Gerrit-HasComments: Yes
