Jim Apple has posted comments on this change. Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null" ......................................................................
Patch Set 3: (8 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-def Done 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? Done 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 w I think this is the same meaning as in the other codegen earlier in this patch that Tim responded to you about. 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 i data() returns a const pointer for strings until C++17, but the other two places I changed. For here, I added a conditional guard to make sure we don't hit the 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? Yes - data() returns a const char * (until c++17), but string_val.ptr is a char *. http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/util/internal-queue-test.cc File be/src/util/internal-queue-test.cc: Line 127: queue.Enqueue(nodes.data()); > These aren't necessary because of the resize aboveand inconsistent with usi Done Line 194: queue.Enqueue(nodes.data()); > Revert this one? It's not necessary and inconsistent with the lines immedia Done Line 202: queue.Enqueue(nodes.data()); > Same here. Done -- 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: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
