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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Tue, 10 Oct 2017 21:00:23 +0000 Gerrit-HasComments: Yes
