[Warns] Reduce warnings amount #523

Devjiu · 2023-06-12T17:58:14Z

This commit is an initial step to remove warnings from build. Currently
it's focused on linux build.

Always_inline attribute warning works incorrectly. In this case we want
to keep symbol and than inline it during target llvm ir optimisation.

See also: #521

Signed-off-by: Dmitrii Makarenko [email protected]

Devjiu · 2023-06-12T18:02:39Z

Replacing with FORCE_INLINE does not help for:

/localdisk/dmitriim/hdk/omniscidb/Utils/ExtractFromTime.cpp:199:30: warning: 'always_inline' function might not be inlinable [-Wattributes]
  199 | ALWAYS_INLINE DEVICE int64_t extract_week(const int64_t timeval) {

In this case looks like we should use simple inline. Is this an acceptable approach?

alexbaden

Can you also remove the FORCE and ALWAYS inline changes for now? I think that will require more thought. Let's tackle the other warnings now and look at inline warnings later.

alexbaden · 2023-06-13T17:49:26Z

omniscidb/Tests/ArrowStorageTest.cpp

-  float64_builder.AppendValues({});
-  float64_builder.Finish(&empty_array);
+  // auto status = float64_builder.AppendValues({});
+  ASSERT_TRUE(float64_builder.AppendValues({}).ok());


There is a macro you can use here -- ARROW_THROW_NOT_OK I believe.

alexbaden · 2023-06-13T17:50:16Z

omniscidb/QueryEngine/Optimization/AnnotateInternalFunctionsPass.h

@@ -37,7 +37,7 @@ class AnnotateInternalFunctionsPass
                              llvm::CGSCCAnalysisManager& AM,
                              llvm::LazyCallGraph& CG,
                              llvm::CGSCCUpdateResult& UR) {
-    bool updated_function_defs = false;
+    [[maybe_unused]] bool updated_function_defs = false;


It looks like this is actually not used? @lmontigny

I confirm, it's not used

alexbaden · 2023-06-13T17:51:10Z

omniscidb/IR/Node.h

+  const std::string& getFieldName(size_t i) const override {
+    CHECK(false);
+    // just a stub, shouldn't be reached.
+    return op_type_;


Recommend returning empty string "" instead of a member var, since the return is not meaningful b/c the function is not implemented.

Returning a reference to a local object would produce another warning though. I wonder if we can modify CHECK in such a way that the compiler would understand what we actually call abort. Or maybe use some new macro like FAIL to avoid unreachable return statements.

Devjiu · 2023-06-14T17:45:17Z

Can you also remove the FORCE and ALWAYS inline changes for now? I think that will require more thought. Let's tackle the other warnings now and look at inline warnings later.

Sure, It was an initial approach. AFAIU Force inline removes symbol for this function and we can't call it on runtime, so many tests are failed with forced. Looks like warning generated by gcc is incorrect for our case - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55830 . So I just suppressed it.

alexbaden · 2023-06-18T20:57:55Z

CMakeLists.txt

@@ -15,6 +15,7 @@ project(

 set(CMAKE_CXX_STANDARD 17)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
+set(CMAKE_SUPPRESS_DEVELOPER_WARNINGS 1 CACHE INTERNAL "No dev warnings")


I was able to fix the cmake warning here: #531

Cool) I also tried in the same way, but didn't assume that the extra space would resolve it.

If the warning is fixed we need to remove this item.

alexbaden · 2023-06-18T20:58:37Z

omniscidb/IR/Node.h

-  const std::string& getFieldName(size_t i) const override { CHECK(false); }
+  const std::string& getFieldName(size_t i) const override {
+    CHECK(false);
+    // Check(false) macro have abort in Logger d-tor,


Let's just return an empty string, nobody will ever actually access it (b/c of the CHECK), and that is consistent with the rest of the code base.

Sorry, but signature with const ref don't allow to return "" here. I checked and get returning address of local variable or temporary or something like this, I can WA with static const std::string.

You can also throw an exception instead of return to resolve the warning.

Ok, I guess we can leave the abort if it builds fine w/ gcc and clang.

alexbaden · 2023-06-21T22:47:13Z

CMakeLists.txt

@@ -15,6 +15,7 @@ project(

 set(CMAKE_CXX_STANDARD 17)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
+set(CMAKE_SUPPRESS_DEVELOPER_WARNINGS 1 CACHE INTERNAL "No dev warnings")


If the warning is fixed we need to remove this item.

alexbaden · 2023-06-21T23:03:18Z

omniscidb/IR/Node.h

-  const std::string& getFieldName(size_t i) const override { CHECK(false); }
+  const std::string& getFieldName(size_t i) const override {
+    CHECK(false);
+    // Check(false) macro have abort in Logger d-tor,


Ok, I guess we can leave the abort if it builds fine w/ gcc and clang.

alexbaden · 2023-06-26T22:29:49Z

@Devjiu can you address #523 (comment) so we can merge this? It would be nice to suppress some of these warnings.

This commit is an initial step to remove warnings from build. Currently it's focused on linux gcc configuration. Always_inline attribute warning works incorrectly. In this case we want to keep symbol and than inline it during target llvm ir optimisation. See also: #521 Signed-off-by: Dmitrii Makarenko <[email protected]>

Devjiu requested review from kurapov-peter and ienkovich and removed request for kurapov-peter June 12, 2023 17:58

alexbaden suggested changes Jun 13, 2023

View reviewed changes

Devjiu force-pushed the dmitriim/add_return_stmnt branch 2 times, most recently from a6590f5 to 6d4c150 Compare June 14, 2023 17:39

alexbaden suggested changes Jun 18, 2023

View reviewed changes

alexbaden suggested changes Jun 21, 2023

View reviewed changes

Devjiu force-pushed the dmitriim/add_return_stmnt branch 2 times, most recently from d91de20 to becdd8c Compare June 30, 2023 13:21

Devjiu marked this pull request as ready for review June 30, 2023 13:21

Devjiu force-pushed the dmitriim/add_return_stmnt branch from becdd8c to bf845d8 Compare June 30, 2023 14:46

alexbaden approved these changes Jun 30, 2023

View reviewed changes

kurapov-peter approved these changes Jul 3, 2023

View reviewed changes

kurapov-peter merged commit 9d3fcf0 into main Jul 3, 2023

kurapov-peter deleted the dmitriim/add_return_stmnt branch July 3, 2023 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Warns] Reduce warnings amount #523

[Warns] Reduce warnings amount #523

Devjiu commented Jun 12, 2023 •

edited

Loading

Devjiu commented Jun 12, 2023

alexbaden left a comment

alexbaden Jun 13, 2023

alexbaden Jun 13, 2023

lmontigny Jun 14, 2023

alexbaden Jun 13, 2023

ienkovich Jun 13, 2023

Devjiu commented Jun 14, 2023

alexbaden Jun 18, 2023

Devjiu Jun 19, 2023

alexbaden Jun 21, 2023

alexbaden Jun 18, 2023

Devjiu Jun 19, 2023

ienkovich Jun 20, 2023

alexbaden Jun 21, 2023

alexbaden Jun 21, 2023

alexbaden Jun 21, 2023

alexbaden commented Jun 26, 2023

[Warns] Reduce warnings amount #523

[Warns] Reduce warnings amount #523

Conversation

Devjiu commented Jun 12, 2023 • edited Loading

Devjiu commented Jun 12, 2023

alexbaden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Devjiu commented Jun 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbaden commented Jun 26, 2023

Devjiu commented Jun 12, 2023 •

edited

Loading