Lars Volker has posted comments on this change. Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null" ......................................................................
Patch Set 3: (9 comments) Thank you for fixing this. Please see my inline comments. http://gerrit.cloudera.org:8080/#/c/7008/3//COMMIT_MSG Commit Message: Line 22: You could add a link to http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#464 which mentions that this was one of the reasons for introducing data(). http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: Line 731: // ctx_vector = build_expr_ctxs_.data() / ctx_vector = probe_expr_ctxs_.data() I'm not sure why this line is here at all. Should we remove it? Line 1093: // ctx_vector = build_expr_ctxs_.data() same here? http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/hdfs-text-scanner.cc File be/src/exec/hdfs-text-scanner.cc: Line 379: row_end_locations_.data(), Do you want to fix the line wrapping while you're here? http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/old-hash-table.cc File be/src/exec/old-hash-table.cc: Line 301: // Load build_expr_ctxs_.data() / probe_expr_ctxs_.data() I'm not sure that the replacement preserves the comment well. Do you know what the intent is here? Can you rephrase? Line 643: // Load build_expr_ctxs_.data() same here? http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/parquet-column-stats.inline.h File be/src/exec/parquet-column-stats.inline.h: Line 75: v, bytes_needed, reinterpret_cast<uint8_t*>(&(*out)[0])); Out cannot be empty here, but the code looks suspicious. Should we change it? There are two other places with similar code: git grep "&(\*\([A-Za-z0-9_]\+\))\[0\]" be/src/exec/hdfs-parquet-scanner-ir.cc:30: ExprContext* const* conjunct_ctxs = &(*scanner_conjunct_ctxs_)[0]; be/src/exec/hdfs-scanner.h:366: return ExecNode::EvalConjuncts(&(*scanner_conjunct_ctxs_)[0], be/src/exec/parquet-column-stats.inline.h:75: v, bytes_needed, reinterpret_cast<uint8_t*>(&(*out)[0])); http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/experiments/tuple-splitter-test.cc File be/src/experiments/tuple-splitter-test.cc: PS3, Line 284: split_counts_ Will this work if split_counts_ is empty()? Should we have a comment to explain why this is not UB? http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exprs/expr-value.h File be/src/exprs/expr-value.h: Line 72: string_val.ptr = string_val.len > 0 ? &string_data[0] : nullptr; Nice. Does this get us around the const cast? -- To view, visit http://gerrit.cloudera.org:8080/7008 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
