search

utelle / wxpdfdoc

wxPdfDocument - Generation of PDF documents from wxWidgets applications
http://utelle.github.io/wxpdfdoc/
Other
70 stars 27 forks source link

Fix clang warning about inconsistent use of override #76

Closed vadz closed 3 years ago

vadz commented 3 years ago

Use it for wxPdfDCImpl::GetResolution() too, as this virtual function exists (since a very long time) in the base wxDCImpl class too.

I'm not sure how did I manage to not notice this until now, but building with clang resulted in multiple instances of

In file included from $wxpdfdoc/samples/pdfdc/printing.cpp:45:
$wxpdfdoc/include/wx/pdfdc.h:83:7: warning: 'GetResolution' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
  int GetResolution() const;
      ^
$wx/include/wx/dc.h:667:17: note: overridden virtual function is here
    virtual int GetResolution() const
                ^

which are fixed by this trivial change.

utelle commented 3 years ago

I'm not sure how did I manage to not notice this until now, but building with clang resulted in multiple instances of [...] GetResolution

IMHO this can be easily explained: wxPdfDocument's Travis CI runs against version 3.0.5 of wxWidgets on macOS, which does not provoke this warning. So it went undetected.

This clearly indicates that CI against a more recent wxWidgets version, or even wxWidgets master, would be useful to detect potential problems earlier. Advice how to accomplish this would be highly appreciated.

vadz commented 3 years ago

I'm not sure how did I manage to not notice this until now, but building with clang resulted in multiple instances of [...] GetResolution

IMHO this can be easily explained: wxPdfDocument's Travis CI runs against version 3.0.5 of wxWidgets on macOS, which does not provoke this warning. So it went undetected.

But wx 3.0 does have virtual int GetResolution() const in wxDCImpl already, which is why I was surprised.

This clearly indicates that CI against a more recent wxWidgets version, or even wxWidgets master, would be useful to detect potential problems earlier. Advice how to accomplish this would be highly appreciated.

I still have "setup GitHub Actions for wxPdfDoc" in my TODO list... I just don't know when will I get to it, sorry :-(

Thanks for merging this!

utelle commented 3 years ago

But wx 3.0 does have virtual int GetResolution() const in wxDCImpl already, which is why I was surprised.

That's really strange, indeed. I have no clue why the CI with clang based on wxMac 3.0.5 does not show warnings.

I still have "setup GitHub Actions for wxPdfDoc" in my TODO list... I just don't know when will I get to it, sorry :-(

In fact, wxPdfDocument is my component, and I should take care of this issue myself. I will try to look into it in the near future.

Thanks for merging this!

You are welcome. I really appreciate that you provide PRs to fix such issues. Thanks.