[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 7: Thanks for the contribution! -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 7 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Nov 2017 21:57:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 6: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 6 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Nov 2017 21:50:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. IMPALA-6084: Avoid using of global namespace for llvm There are a large number of places in the backend where we import everything from the llvm namespace into the global namespace. (e.g. using namespace llvm;) Here are the reasons why we don't prefer this: * It could make symbol conflicts if a newly added code has same symbole name. * It makes code readability uncomfortable. We may not recognize a symbol came from. We adopt a sequence of namespace specifiers in each use case. Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Reviewed-on: http://gerrit.cloudera.org:8080/8489 Reviewed-by: Tim ArmstrongTested-by: Impala Public Jenkins --- M be/src/benchmarks/hash-benchmark.cc M be/src/codegen/codegen-anyval.cc M be/src/codegen/codegen-callgraph.cc M be/src/codegen/codegen-symbol-emitter.cc M be/src/codegen/codegen-util.cc M be/src/codegen/instruction-counter-test.cc M be/src/codegen/instruction-counter.cc M be/src/codegen/llvm-codegen-test.cc M be/src/codegen/llvm-codegen.cc M be/src/codegen/llvm-codegen.h M be/src/exec/blocking-join-node.cc M be/src/exec/exec-node.cc M be/src/exec/filter-context.cc M be/src/exec/hash-table.cc M be/src/exec/hdfs-avro-scanner.cc M be/src/exec/hdfs-parquet-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scanner.cc M be/src/exec/hdfs-sequence-scanner.cc M be/src/exec/hdfs-text-scanner.cc M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/select-node.cc M be/src/exec/text-converter.cc M be/src/exec/topn-node.cc M be/src/exec/union-node.cc M be/src/exprs/agg-fn-evaluator.cc M be/src/exprs/agg-fn.cc M be/src/exprs/case-expr.cc M be/src/exprs/compound-predicates.cc M be/src/exprs/expr-codegen-test.cc M be/src/exprs/expr-test.cc M be/src/exprs/literal.cc M be/src/exprs/null-literal.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-fn-call.cc M be/src/exprs/slot-ref.cc M be/src/runtime/descriptors.cc M be/src/runtime/runtime-state.cc M be/src/runtime/types.cc M be/src/util/tuple-row-compare.cc 42 files changed, 1,417 insertions(+), 1,384 deletions(-) Approvals: Tim Armstrong: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 7 Gerrit-Owner: Kim Jin Chul Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 6: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1475/ -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 6 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Nov 2017 18:24:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 6 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Nov 2017 18:23:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 5: (2 comments) I had one nit but just fixed it myself since I don't think it's worth doing another iteration to fix. http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-callgraph.cc File be/src/codegen/codegen-callgraph.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-callgraph.cc@41 PS4, Line 41: for (llvm::Use& u: val->uses()) > I think we usually stick to using no space between ":" like IN the backend code, clang-format likes to put spaces on both sides, so that's what we've standardised on. The frontend code only has a trailing space. It doesn't really matter much though. http://gerrit.cloudera.org:8080/#/c/8489/5/be/src/exprs/scalar-expr.cc File be/src/exprs/scalar-expr.cc: http://gerrit.cloudera.org:8080/#/c/8489/5/be/src/exprs/scalar-expr.cc@260 PS5, Line 260: // Compute the alignment of this value. Values should be self-aligned for This formatting is weird - the old formatting was better. -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 5 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Nov 2017 18:23:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Tim Armstrong has uploaded a new patch set (#6) to the change originally created by Kim Jin Chul. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. IMPALA-6084: Avoid using of global namespace for llvm There are a large number of places in the backend where we import everything from the llvm namespace into the global namespace. (e.g. using namespace llvm;) Here are the reasons why we don't prefer this: * It could make symbol conflicts if a newly added code has same symbole name. * It makes code readability uncomfortable. We may not recognize a symbol came from. We adopt a sequence of namespace specifiers in each use case. Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a --- M be/src/benchmarks/hash-benchmark.cc M be/src/codegen/codegen-anyval.cc M be/src/codegen/codegen-callgraph.cc M be/src/codegen/codegen-symbol-emitter.cc M be/src/codegen/codegen-util.cc M be/src/codegen/instruction-counter-test.cc M be/src/codegen/instruction-counter.cc M be/src/codegen/llvm-codegen-test.cc M be/src/codegen/llvm-codegen.cc M be/src/codegen/llvm-codegen.h M be/src/exec/blocking-join-node.cc M be/src/exec/exec-node.cc M be/src/exec/filter-context.cc M be/src/exec/hash-table.cc M be/src/exec/hdfs-avro-scanner.cc M be/src/exec/hdfs-parquet-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scanner.cc M be/src/exec/hdfs-sequence-scanner.cc M be/src/exec/hdfs-text-scanner.cc M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/select-node.cc M be/src/exec/text-converter.cc M be/src/exec/topn-node.cc M be/src/exec/union-node.cc M be/src/exprs/agg-fn-evaluator.cc M be/src/exprs/agg-fn.cc M be/src/exprs/case-expr.cc M be/src/exprs/compound-predicates.cc M be/src/exprs/expr-codegen-test.cc M be/src/exprs/expr-test.cc M be/src/exprs/literal.cc M be/src/exprs/null-literal.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-fn-call.cc M be/src/exprs/slot-ref.cc M be/src/runtime/descriptors.cc M be/src/runtime/runtime-state.cc M be/src/runtime/types.cc M be/src/util/tuple-row-compare.cc 42 files changed, 1,417 insertions(+), 1,384 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/8489/6 -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 6 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-anyval.cc@156 PS4, Line 156: // DecimalVal, > you can fit this in the previous line Done http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-callgraph.cc File be/src/codegen/codegen-callgraph.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-callgraph.cc@41 PS4, Line 41: for (llvm::Use& u : val->uses()) > I think we usually stick to using no space between ":" like Done for removal of the whitespace http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/instruction-counter.cc File be/src/codegen/instruction-counter.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/instruction-counter.cc@43 PS4, Line 43: // Create all instruction counter and put them into counters_. Any : // InstructionCount that has instructions delegated to it in : // InstructionCounter::visit(const Instruction ) > can fit in 2 lines Done http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/llvm-codegen.cc@1127 PS4, Line 1127: llvm:: > search/replace gone wrong Done http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/hash-table.cc@40 PS4, Line 40: using llvm::APFloat; : using llvm::ArrayRef; : using llvm::BasicBlock; : using llvm::ConstantFP; : using llvm::Function; : using llvm::LLVMContext; : using llvm::PHINode; : using llvm::PointerType; : using llvm::Type; : using llvm::Value; > you can remove these Done http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-builder.cc File be/src/exec/partitioned-hash-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-builder.cc@49 PS4, Line 49: using llvm::ConstantInt; : using llvm::Function; : using llvm::LLVMContext; : using llvm::PointerType; : using llvm::Type; : using llvm::Value > you can remove these Done http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-node.cc@50 PS4, Line 50: using llvm::BasicBlock; : using llvm::ConstantInt; : using llvm::Function; : using llvm::GlobalValue; : using llvm::LLVMContext; : using llvm::PointerType; : using llvm::Type; : using llvm::Value; > you can remove these Done http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-node.cc@131 PS4, Line 131: llvm:: > search/replace gone wrong Done http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/select-node.cc File be/src/exec/select-node.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/select-node.cc@30 PS4, Line 30: using llvm::Function; > you can remove this Done http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-expr.cc File be/src/exprs/scalar-expr.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-expr.cc@260 PS4, Line 260: llvm:: > search/replace gone wrong, after you fix that, you can fit this into one li Done http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-fn-call.cc@46 PS4, Line 46: using llvm::ArrayType; : using llvm::BasicBlock; : using llvm::Function; : using llvm::GlobalVariable; : using llvm::PointerType; : using llvm::Type; : using llvm::Value; > you can remove these Done http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/runtime/descriptors.cc@40 PS4, Line 40: using llvm::Constant; : using llvm::ConstantAggregateZero; : using llvm::ConstantInt; : using llvm::ConstantStruct; : using llvm::StructType; > you can remove these Done -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id:
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Hello Philip Zeyliger, Tim Armstrong, Bikramjeet Vig, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8489 to look at the new patch set (#5). Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. IMPALA-6084: Avoid using of global namespace for llvm There are a large number of places in the backend where we import everything from the llvm namespace into the global namespace. (e.g. using namespace llvm;) Here are the reasons why we don't prefer this: * It could make symbol conflicts if a newly added code has same symbole name. * It makes code readability uncomfortable. We may not recognize a symbol came from. We adopt a sequence of namespace specifiers in each use case. Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a --- M be/src/benchmarks/hash-benchmark.cc M be/src/codegen/codegen-anyval.cc M be/src/codegen/codegen-callgraph.cc M be/src/codegen/codegen-symbol-emitter.cc M be/src/codegen/codegen-util.cc M be/src/codegen/instruction-counter-test.cc M be/src/codegen/instruction-counter.cc M be/src/codegen/llvm-codegen-test.cc M be/src/codegen/llvm-codegen.cc M be/src/codegen/llvm-codegen.h M be/src/exec/blocking-join-node.cc M be/src/exec/exec-node.cc M be/src/exec/filter-context.cc M be/src/exec/hash-table.cc M be/src/exec/hdfs-avro-scanner.cc M be/src/exec/hdfs-parquet-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scanner.cc M be/src/exec/hdfs-sequence-scanner.cc M be/src/exec/hdfs-text-scanner.cc M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/select-node.cc M be/src/exec/text-converter.cc M be/src/exec/topn-node.cc M be/src/exec/union-node.cc M be/src/exprs/agg-fn-evaluator.cc M be/src/exprs/agg-fn.cc M be/src/exprs/case-expr.cc M be/src/exprs/compound-predicates.cc M be/src/exprs/expr-codegen-test.cc M be/src/exprs/expr-test.cc M be/src/exprs/literal.cc M be/src/exprs/null-literal.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-fn-call.cc M be/src/exprs/slot-ref.cc M be/src/runtime/descriptors.cc M be/src/runtime/runtime-state.cc M be/src/runtime/types.cc M be/src/util/tuple-row-compare.cc 42 files changed, 1,419 insertions(+), 1,385 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/8489/5 -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 5 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 4: Code-Review+1 (12 comments) Looks good to me, just some cleanup left to do http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-anyval.cc@156 PS4, Line 156: // DecimalVal, you can fit this in the previous line http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-callgraph.cc File be/src/codegen/codegen-callgraph.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-callgraph.cc@41 PS4, Line 41: for (llvm::Use& u : val->uses()) I think we usually stick to using no space between ":" like "for (llvm::Use& u: val->uses())" but I see exceptions in the codebase to this rule. Can someone please confirm? http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/instruction-counter.cc File be/src/codegen/instruction-counter.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/instruction-counter.cc@43 PS4, Line 43: // Create all instruction counter and put them into counters_. Any : // InstructionCount that has instructions delegated to it in : // InstructionCounter::visit(const Instruction ) can fit in 2 lines http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/llvm-codegen.cc@1127 PS4, Line 1127: llvm:: search/replace gone wrong http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/hash-table.cc@40 PS4, Line 40: using llvm::APFloat; : using llvm::ArrayRef; : using llvm::BasicBlock; : using llvm::ConstantFP; : using llvm::Function; : using llvm::LLVMContext; : using llvm::PHINode; : using llvm::PointerType; : using llvm::Type; : using llvm::Value; you can remove these http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-builder.cc File be/src/exec/partitioned-hash-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-builder.cc@49 PS4, Line 49: using llvm::ConstantInt; : using llvm::Function; : using llvm::LLVMContext; : using llvm::PointerType; : using llvm::Type; : using llvm::Value you can remove these http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-node.cc@50 PS4, Line 50: using llvm::BasicBlock; : using llvm::ConstantInt; : using llvm::Function; : using llvm::GlobalValue; : using llvm::LLVMContext; : using llvm::PointerType; : using llvm::Type; : using llvm::Value; you can remove these http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-node.cc@131 PS4, Line 131: llvm:: search/replace gone wrong http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/select-node.cc File be/src/exec/select-node.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/select-node.cc@30 PS4, Line 30: using llvm::Function; you can remove this http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-expr.cc File be/src/exprs/scalar-expr.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-expr.cc@260 PS4, Line 260: llvm:: search/replace gone wrong, after you fix that, you can fit this into one line http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-fn-call.cc@46 PS4, Line 46: using llvm::ArrayType; : using llvm::BasicBlock; : using llvm::Function; : using llvm::GlobalVariable; : using llvm::PointerType; : using llvm::Type; : using llvm::Value; you can remove these http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/runtime/descriptors.cc@40 PS4, Line 40: using llvm::Constant; : using llvm::ConstantAggregateZero; : using llvm::ConstantInt; : using llvm::ConstantStruct; : using llvm::StructType; you can remove these -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 4: > I'm ok with using llvm:: in those files, but was going to let > Bikram also weigh in. I agree with Tim and am ok with using llvm:: as well. I was waiting to make a complete pass before I replied, as I initially noticed many search/replace gone wrong, but seems like you fixed those in the latest patch. Let me take another pass to see if i missed something else. -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 4 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 10 Nov 2017 00:56:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 4: I'm ok with using llvm:: in those files, but was going to let Bikram also weigh in. -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 4 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 10 Nov 2017 00:41:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 4: (2 comments) @Tim and Bikram, both of you wants rollback the changes for only be/src/codegen due to llvm's symbol change in the future. Do I proceed it? Or do you need further discussion? http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/codegen-anyval.cc@479 PS3, Line 479: casts it b > search-and-replace gone wrong Fixed unintentional changes http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/instruction-counter.cc File be/src/codegen/instruction-counter.cc: http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/instruction-counter.cc@28 PS3, Line 28: const char* InstructionCounter::TOTAL_INSTS = "Total Instructions"; > This was probably not intentional? Fixed unintentional changes -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 4 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 09 Nov 2017 02:15:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Hello Philip Zeyliger, Tim Armstrong, Bikramjeet Vig, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8489 to look at the new patch set (#4). Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. IMPALA-6084: Avoid using of global namespace for llvm There are a large number of places in the backend where we import everything from the llvm namespace into the global namespace. (e.g. using namespace llvm;) Here are the reasons why we don't prefer this: * It could make symbol conflicts if a newly added code has same symbole name. * It makes code readability uncomfortable. We may not recognize a symbol came from. We adopt a sequence of namespace specifiers in each use case. Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a --- M be/src/benchmarks/hash-benchmark.cc M be/src/codegen/codegen-anyval.cc M be/src/codegen/codegen-callgraph.cc M be/src/codegen/codegen-symbol-emitter.cc M be/src/codegen/codegen-util.cc M be/src/codegen/instruction-counter-test.cc M be/src/codegen/instruction-counter.cc M be/src/codegen/llvm-codegen-test.cc M be/src/codegen/llvm-codegen.cc M be/src/codegen/llvm-codegen.h M be/src/exec/blocking-join-node.cc M be/src/exec/exec-node.cc M be/src/exec/filter-context.cc M be/src/exec/hash-table.cc M be/src/exec/hdfs-avro-scanner.cc M be/src/exec/hdfs-parquet-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scanner.cc M be/src/exec/hdfs-sequence-scanner.cc M be/src/exec/hdfs-text-scanner.cc M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/select-node.cc M be/src/exec/text-converter.cc M be/src/exec/topn-node.cc M be/src/exec/union-node.cc M be/src/exprs/agg-fn-evaluator.cc M be/src/exprs/agg-fn.cc M be/src/exprs/case-expr.cc M be/src/exprs/compound-predicates.cc M be/src/exprs/expr-codegen-test.cc M be/src/exprs/expr-test.cc M be/src/exprs/literal.cc M be/src/exprs/null-literal.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-fn-call.cc M be/src/exprs/slot-ref.cc M be/src/runtime/descriptors.cc M be/src/runtime/runtime-state.cc M be/src/runtime/types.cc M be/src/util/tuple-row-compare.cc 42 files changed, 1,420 insertions(+), 1,346 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/8489/4 -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 4 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 3: @Bikram I think that would be convenient, except LLVM has a lot of stuff in its namespace and it seems to change from release to release. E.g. one thing that motivated this was that LLVM defines a make_unique() function in its namespace, which conflicts with the std:: one. -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 3 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 09 Nov 2017 01:15:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/instruction-counter.cc File be/src/codegen/instruction-counter.cc: http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/instruction-counter.cc@28 PS3, Line 28: const char* InstructionCounter::TOTAL_INSTS = "Total llvm::Instructions"; This was probably not intentional? -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 3 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 08 Nov 2017 22:41:34 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 3: does it make sense to import llvm namespace globally for the files under "be/src/codegen/" since they primarily work with code from that namespace? that might help avoid code bloat for those files -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 3 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 08 Nov 2017 21:41:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. Patch Set 3: (1 comment) I started looking through the first few files and it looks good to me so far. Maybe Bikram can continue the review though when he has time. http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/codegen-anyval.cc@479 PS3, Line 479: llvm::cast search-and-replace gone wrong -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 3 Gerrit-Owner: Kim Jin ChulGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 08 Nov 2017 18:52:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm
Kim Jin Chul has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/8489 ) Change subject: IMPALA-6084: Avoid using of global namespace for llvm .. IMPALA-6084: Avoid using of global namespace for llvm There are a large number of places in the backend where we import everything from the llvm namespace into the global namespace. (e.g. using namespace llvm;) Here are the reasons why we don't prefer this: * It could make symbol conflicts if a newly added code has same symbole name. * It makes code readability uncomfortable. We may not recognize a symbol came from. We adopt a sequence of namespace specifiers in each use case. Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a --- M be/src/benchmarks/hash-benchmark.cc M be/src/codegen/codegen-anyval.cc M be/src/codegen/codegen-callgraph.cc M be/src/codegen/codegen-symbol-emitter.cc M be/src/codegen/codegen-util.cc M be/src/codegen/instruction-counter-test.cc M be/src/codegen/instruction-counter.cc M be/src/codegen/llvm-codegen-test.cc M be/src/codegen/llvm-codegen.cc M be/src/exec/blocking-join-node.cc M be/src/exec/exec-node.cc M be/src/exec/filter-context.cc M be/src/exec/hash-table.cc M be/src/exec/hdfs-avro-scanner.cc M be/src/exec/hdfs-parquet-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scanner.cc M be/src/exec/hdfs-sequence-scanner.cc M be/src/exec/hdfs-text-scanner.cc M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/select-node.cc M be/src/exec/text-converter.cc M be/src/exec/topn-node.cc M be/src/exec/union-node.cc M be/src/exprs/agg-fn-evaluator.cc M be/src/exprs/agg-fn.cc M be/src/exprs/case-expr.cc M be/src/exprs/compound-predicates.cc M be/src/exprs/expr-codegen-test.cc M be/src/exprs/expr-test.cc M be/src/exprs/literal.cc M be/src/exprs/null-literal.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-fn-call.cc M be/src/exprs/slot-ref.cc M be/src/runtime/descriptors.cc M be/src/runtime/runtime-state.cc M be/src/runtime/types.cc M be/src/util/tuple-row-compare.cc 41 files changed, 1,452 insertions(+), 1,378 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/8489/3 -- To view, visit http://gerrit.cloudera.org:8080/8489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a Gerrit-Change-Number: 8489 Gerrit-PatchSet: 3 Gerrit-Owner: Kim Jin Chul