Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: fix gcc/clang warnings #297

Closed
wants to merge 1 commit into from
Closed

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Sep 24, 2019

2 warnings still here. maybe-uninitialized looks like GCC bug. unused-but-set-variable introduced in b53010b#diff-86a9160147028a03800f8ba4fa555285R1502-R1525 -- not sure how should be handled.

PR GCC (9.2.1):

../src/llv8.cc: In member function ‘llnode::v8::Value llnode::v8::JSObject::GetDescriptorProperty(std::string, llnode::v8::Map, llnode::Error&)’:
../src/llv8.cc:1149:14: warning: variable ‘value’ set but not used [-Wunused-but-set-variable]
 1149 |       double value;
      |              ^~~~~
  CXX(target) Release/obj.target/plugin/src/llv8-constants.o
  CXX(target) Release/obj.target/plugin/src/llscan.o
../src/llscan.cc: In member function ‘virtual bool llnode::FindReferencesCmd::DoExecute(lldb::SBDebugger, char**, lldb::SBCommandReturnObject&)’:
../src/llscan.cc:568:22: warning: ‘scanner’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  568 |     ScanForReferences(scanner);
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~

master GCC (9.2.1):

../src/llv8.cc: In member function ‘double llnode::v8::LLV8::LoadDouble(int64_t, llnode::Error&)’:
../src/llv8.cc:114:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  114 |   return *reinterpret_cast<double*>(&value);
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/llv8.cc: In member function ‘llnode::v8::Value llnode::v8::JSObject::GetDescriptorProperty(std::string, llnode::v8::Map, llnode::Error&)’:
../src/llv8.cc:1149:14: warning: variable ‘value’ set but not used [-Wunused-but-set-variable]
 1149 |       double value;
      |              ^~~~~
../src/llv8.cc: In constructor ‘llnode::v8::StackTrace::StackTrace(llnode::v8::JSArray, llnode::Error&)’:
../src/llv8.cc:1291:74: warning: format ‘%lld’ expects argument of type ‘long long int’, but argument 2 has type ‘int’ [-Wformat=]
 1291 |           "JSArray doesn't look like a Stack Frames array. stack_len: %lld "
      |                                                                       ~~~^
      |                                                                          |
      |                                                                          long long int
      |                                                                       %d
 1292 |           "array_len: %lld",
 1293 |           len_, frame_array_.GetArrayLength(err));
      |           ~~~~                                                            
      |           |
      |           int
../src/llv8.cc:1292:26: warning: format ‘%lld’ expects argument of type ‘long long int’, but argument 3 has type ‘int64_t’ {aka ‘long int’} [-Wformat=]
 1292 |           "array_len: %lld",
      |                       ~~~^
      |                          |
      |                          long long int
      |                       %ld
 1293 |           len_, frame_array_.GetArrayLength(err));
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                            |
      |                                            int64_t {aka long int}
../src/llscan.cc: In member function ‘virtual bool llnode::FindReferencesCmd::DoExecute(lldb::SBDebugger, char**, lldb::SBCommandReturnObject&)’:
../src/llscan.cc:568:22: warning: ‘scanner’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  568 |     ScanForReferences(scanner);
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~

master clang (8.0.0):

In file included from ../src/llnode.cc:13:
../src/llnode.h:42:13: warning: private field 'llv8_' is not used [-Wunused-private-field]
  v8::LLV8* llv8_;
            ^
1 warning generated.
../src/llv8.cc:1293:11: warning: format specifies type 'long long' but the argument has type 'int' [-Wformat]
          len_, frame_array_.GetArrayLength(err));
          ^~~~
../src/llv8.cc:1293:17: warning: format specifies type 'long long' but the argument has type 'int64_t' (aka 'long') [-Wformat]
          len_, frame_array_.GetArrayLength(err));
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
../src/llscan.cc:650:19: warning: format string is not a string literal (potentially insecure) [-Wformat-security]
    result.Printf(seen_str.str().c_str());
                  ^~~~~~~~~~~~~~~~~~~~~~
../src/llscan.cc:650:19: note: treat the string as an argument to avoid this
    result.Printf(seen_str.str().c_str());
                  ^
                  "%s",
1 warning generated.

@codecov-io
Copy link

Codecov Report

Merging #297 into master will increase coverage by 0.61%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   79.23%   79.85%   +0.61%     
==========================================
  Files          33       33              
  Lines        4229     4229              
==========================================
+ Hits         3351     3377      +26     
+ Misses        878      852      -26
Impacted Files Coverage Δ
src/llnode.cc 74.2% <ø> (ø) ⬆️
src/llnode.h 50% <ø> (ø) ⬆️
src/llscan.cc 61.31% <100%> (ø) ⬆️
src/llv8.cc 75.46% <100%> (+2.46%) ⬆️
src/llv8-constants.cc 83.49% <0%> (+0.97%) ⬆️
src/llv8-inl.h 92.67% <0%> (+1.04%) ⬆️
src/llv8-constants.h 100% <0%> (+1.51%) ⬆️
src/llv8.h 100% <0%> (+2.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1a74b0...8fe0f04. Read the comment docs.

@mmarchini
Copy link
Contributor

Thanks for looking into it! Overall LGTM.

maybe-uninitialized looks like GCC bug

It does indeed. Maybe there's a better way to write that piece of code which will prevent the warning though? I'll open an issue.

unused-but-set-variable introduced in b53010b#diff-86a9160147028a03800f8ba4fa555285R1502-R1525 -- not sure how should be handled.

That looks wrong... I'm pretty sure I stumbled on this piece of code a few times and didn't change for a reason. I'll open an issue to investigate it further.

mmarchini pushed a commit that referenced this pull request Sep 25, 2019
PR-URL: #297
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@mmarchini
Copy link
Contributor

Landed in 84eefb4, thanks!

@mmarchini mmarchini closed this Sep 25, 2019
@fanatid fanatid deleted the fix-warnings branch September 26, 2019 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants