Fix more build warnings (#1616)

[marcalff]

Fixes #1616

Changes

Fixed the following warnings reported by gcc:

-Wall
-Werror
-Wextra-semi
-Werror=unused-parameter
-Werror=undef
-Werror=overloaded-virtual
-Werror=ignored-qualifiers

In particular, gcc gets confused by the trace::Span::AddEvent() methods,
and reports an error with -Werror=overloaded-virtual.

Likewise for logs::Logger::Log()

Fixed the following warnings reported by clang:

[-Werror,-Wimplicit-exception-spec-mismatch]

[-Werror,-Winconsistent-missing-override]

[-Werror,-Wpessimizing-move]

[-Werror,-Wimplicit-const-int-float-conversion]

[-Werror,-Wdelete-non-abstract-non-virtual-dtor]

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #1620 (79c53f9) into main (a7c4bbc) will decrease coverage by 0.10%.
The diff coverage is 56.25%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1620      +/-   ##
==========================================
- Coverage   85.22%   85.12%   -0.09%     
==========================================
  Files         156      159       +3     
  Lines        4978     4999      +21     
==========================================
+ Hits         4242     4255      +13     
- Misses        736      744       +8     

Impacted Files	Coverage Δ
...pi/include/opentelemetry/context/runtime_context.h	97.60% <ø> (ø)
...etry/sdk/metrics/aggregation/default_aggregation.h	58.19% <0.00%> (ø)
sdk/include/opentelemetry/sdk/trace/recordable.h	68.75% <0.00%> (-22.91%)	⬇️
sdk/src/trace/span.cc	89.66% <0.00%> (-5.46%)	⬇️
sdk/src/trace/span.h	100.00% <ø> (ø)
api/include/opentelemetry/trace/default_span.h	13.34% <10.00%> (ø)
...opentelemetry/sdk/metrics/view/predicate_factory.h	77.78% <50.00%> (ø)
.../include/opentelemetry/metrics/async_instruments.h	100.00% <100.00%> (ø)
api/include/opentelemetry/metrics/noop.h	40.75% <100.00%> (+1.12%)	⬆️
...pi/include/opentelemetry/metrics/observer_result.h	100.00% <100.00%> (ø)
... and 15 more

[esigo]

could you please change ctor and dtors you added?

  ctor() {}
  virtual ~dtor() {}

to

  ctor() = default;
  virtual ~dtor() = default;

[marcalff]

could you please change ctor and dtors you added?

  ctor() {}
  virtual ~dtor() {}

to

  ctor() = default;
  virtual ~dtor() = default;

Done.

[esigo]

LGTM
Thanks for the fix :)

[lalitb]

In particular, gcc gets confused by the trace::Span::AddEvent() methods,
and reports an error with -Werror=overloaded-virtual.

Do we know if this is some bug in gcc, as it makes us redefine all these methods in the derived class.

[marcalff]

In particular, gcc gets confused by the trace::Span::AddEvent() methods,
and reports an error with -Werror=overloaded-virtual.

Do we know if this is some bug in gcc, as it makes us redefine all these methods in the derived class.

For AddEvent():

• there is a method with 2 parameters in the base class that, inlined, calls a method with 3 parameters in the base class
• there are sub classes, reimplementing the method with 3 parameters.

Testing with gcc 10, gcc reports that:

• a method with 3 parameters in a sub class
• hides a method with 2 parameters in the base class

This is surprising, because the prototype is different, so in theory there should be no virtual method hiding another.

After looking at this for a long time, I still can not decide if:

• this is a bug in gcc, which does not sees the prototype as different,
• this is an exceptionally clever interpretation in gcc, which is able to follow inline virtual methods, and finds two paths of execution that can go from AddEvent(2 param) in the base class to AddEvent(3 param) in a sub class, complaining about the ambiguity.

In any case, even if this is a gcc bug, the code should be clear enough so that correct binary is produced by the compiler. At the end of the day, what is executed in production is the binary, not the source code, and we need to avoid surprises there.

The fix implements AddEvent(2 param) in all sub classes, to follow the same pattern already used for every other AddEvent() methods, which will also help maintenance.

From a performance point of view also, I think:

• delegating from AddEvent(2 parameter) to AddEvent(3 parameters) in the base class, making a call to std::chrono::system_clock::now()
• to later land in a no-op implementation of AddEvent(3 parameters) in a sub class
  is unnecessary overhead.

There is no point to call optimistically std::chrono::system_clock::now() in case it might be needed.
Instead, re implementing all methods in sub classes allows each sub class to do only the strict necessary.

[lalitb]

Thanks, @marcalff for the detailed explanation, does making these hidden api methods non-virtual fix the issue? Though I agree with the performance issue you mentioned with multiple function calls and the unnecessary ts calculation, just trying way to avoid definitions of the same method at multiple places.

[marcalff]

Thanks, @marcalff for the detailed explanation, does making these hidden api methods non-virtual fix the issue? Though I agree with the performance issue you mentioned with multiple function calls and the unnecessary ts calculation, just trying way to avoid definitions of the same method at multiple places.

Hi @lalitb

Changing a virtual method to a non virtual is a binary incompatible change I think (the virtual table is different).
It could fix the issue a different way and avoid repetition, but still at the cost of overhead when the timestamp is not needed.

The current fix looks like the best solution so far.

[lalitb]

Changing a virtual method to a non virtual is a binary incompatible change I think (the virtual table is different).

Agree.

[lalitb]

LGTM. Thanks for fixing these warnings.