impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Date Tue, 10 Oct 2017 21:00:23 GMT
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8233 )

Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
......................................................................


Patch Set 2:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@781
PS2, Line 781:   /// Ensures that an attempt to read diagnostic state would reset the object.
Comment is kinda cryptic. Maybe needs some reference to the LLVM DiagnosticHandler API - I
guess the idea of the class is to implement LLVM's DiagnosticHandler API and save the error
information from callbacks.?


http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@784
PS2, Line 784: 
nit: extra blank line.


http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@785
PS2, Line 785:     DiagnosticHandler() : encountered_error_(false) {}
Let's initialise the member variables inline.


http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@787
PS2, Line 787:     /// Returns the error string if an error was encountered and resets the
state in
Can you give some hint about when a caller should call this? I.e. after an LLVM API call that
can fail but returns error info via this mechanism. We probably want to add a TODO and file
a follow-up JIRA to make sure that we're checking for errors everywhere that we need to.


http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@789
PS2, Line 789: string
std::string in a header (I guess there's a rogue "using std::string" somewhere in another
header).


http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@791
PS2, Line 791:     /// Handler function that sets the state on an instance of this class
I think the comments could make the relationship between these two functions more obvious.
E.g. GetErrorString() returns the last error that was reported via DiagnosticHandlerFn()


http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@796
PS2, Line 796:     string error_str_;
Can you briefly document the member variables and the relationship between them? Since we
return an empty string to indicate an error above, is 'encountered_error_' redundant? I.e.
does {true, ""} behave differently to {false, ""}?


http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@796
PS2, Line 796: string
std::string.


http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@799
PS2, Line 799:   /// Maintains state set by diagnostic handler.
Comment is kinda cryptic. It may not be necessary to have both a class and member comment
since there's only one instance of the class.


http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc@1627
PS2, Line 1627:     error_msg.flush();
I feel like we should also log the error at LOG(INFO) level so that it doesn't get swallowed
silently in places where we're not checking for errors yet. These errors should be rare enough
that they won't add too much to the logs.



-- 
To view, visit http://gerrit.cloudera.org:8080/8233
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 21:00:23 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message