Page MenuHomePhabricator

C#: Remove warning about `if constexpr`
AcceptedPublic

Authored by lucas on Apr 1 2020, 2:27 PM.

Details

Reviewers
felipealmeida
Summary

D11434 (already landed) introduced this warning:

[2563/3562] Compiling C++ object 'src/bin..._.._.._bin_eolian_mono_eolian_mono.cc.o'.
In file included from ../src/bin/eolian_mono/eolian_mono.cc:48:
../src/bindings/mono/eolian_mono/../../../bin/eolian_mono/eolian/mono/marshall_annotation.hh: In lambda function:
../src/bindings/mono/eolian_mono/../../../bin/eolian_mono/eolian/mono/marshall_annotation.hh:66:20: warning: ‘if constexpr’ only available with ‘-std=c++17’ or ‘-std=gnu++17’
   66 |                 if constexpr (efl::eolian::grammar::tag_check<direction_context, Context>::value)
      |                    ^~~~~~~~~
In file included from ../src/bin/eolian_mono/eolian_mono.cc:48:
../src/bindings/mono/eolian_mono/../../../bin/eolian_mono/eolian/mono/marshall_annotation.hh: In lambda function:
../src/bindings/mono/eolian_mono/../../../bin/eolian_mono/eolian/mono/marshall_annotation.hh:110:20: warning: ‘if constexpr’ only available with ‘-std=c++17’ or ‘-std=gnu++17’
  110 |                 if constexpr (efl::eolian::grammar::tag_check<direction_context, Context>::value)
      |

It occurs because if constexpr is a c++17 feature so nonexistent at c++11 that is used at EFL. At previous implementation add by D11434 if constexpr should grant at compiling time that a pice of metaprogrammed code would not be generated if it was ill-formed.

This Diff uses template metaprogramming to create a struct if_direction_context to do the same thing that if constexpr should do in that case.

Test Plan

First I show how I compiled this Diff and used Valgrind to test for memory leak. Latter I show some comparison from Valgrind log from D11434, this Diff and master.

  • For the leaking test uncomment the for at return_string_from_virtual and out_string_from_virtual methods at src/tests/efl_mono/Strings.cs test cases;
  • Meson configured with:
meson --prefix=/usr -Dbindings=cxx,mono -Dmono-beta=true
  • All tests run to make sure everything else works - it should pass exatly at everything that master does;
  • sudo ninja -C build install to install this build of EFL system wild to test for memory leak with Valgrind;
  • To run efl-mono-suit.exe:
MONO_PATH=/usr/lib/efl-mono-1 mono build/src/tests/efl_mono/efl-mono-suite.exe
MONO_PATH=/usr/lib/efl-mono-1 Valgrind --tool=memcheck -v --leak-check=full --log-file=<path to log> --smc-check=all --leak-check=full --show-leak-kinds=all --suppressions=<path to mono.supp> mono build/src/tests/efl_mono/efl-mono-suite.exe

Here is a comparison of Heap Summary and Leak Summary section's of Valgring logs, with and without the testcase - uncoommenting the for at return_string_from_virtual and out_string_from_virtual methods at src/tests/efl_mono/Strings.cs test cases.

The Heap Summarys are statistically equal:

  • Master with testcase:
==168084== HEAP SUMMARY:
==168084==     in use at exit: 2,129,024 bytes in 33,114 blocks
==168084==   total heap usage: 1,115,460 allocs, 1,057,593 frees, 427,120,275 bytes allocated
==168084==
==168084== Searching for pointers to 33,446 not-freed blocks
==168084== Checked 25,849,752 bytes
  • This Diff with testcase:
==129535== HEAP SUMMARY:
==129535==     in use at exit: 2,128,048 bytes in 33,102 blocks
==129535==   total heap usage: 1,113,420 allocs, 1,055,565 frees, 425,662,965 bytes allocated
==129535==
==129535== Searching for pointers to 33,434 not-freed blocks
==129535== Checked 25,848,888 bytes
  • Master without the testcase:
==151909== HEAP SUMMARY:
==151909==     in use at exit: 1,430,232 bytes in 13,105 blocks
==151909==   total heap usage: 965,044 allocs, 927,186 frees, 422,857,704 bytes allocated
  • This Diff without the testcase:
==111248== HEAP SUMMARY:
==111248==     in use at exit: 1,429,256 bytes in 13,093 blocks
==111248==   total heap usage: 963,087 allocs, 925,241 frees, 421,485,738 bytes allocated

The Leak Summarys are statistically equal:

  • Master with the testcase:
==168084== LEAK SUMMARY:
==168084==    definitely lost: 108,693 bytes in 6,782 blocks
==168084==    indirectly lost: 14,383 bytes in 219 blocks
==168084==      possibly lost: 72 bytes in 1 blocks
==168084==    still reachable: 1,930,468 bytes in 26,444 blocks
==168084==         suppressed: 0 bytes in 0 blocks
  • This Diff with the testcase:
==129535== LEAK SUMMARY:
==129535==    definitely lost: 108,630 bytes in 6,776 blocks
==129535==    indirectly lost: 14,090 bytes in 211 blocks
==129535==      possibly lost: 0 bytes in 0 blocks
==129535==    still reachable: 1,929,920 bytes in 26,447 blocks
==129535==         suppressed: 0 bytes in 0 blocks
  • Master without the testcase:
==151909== LEAK SUMMARY:
==151909==    definitely lost: 108,568 bytes in 6,773 blocks
==151909==    indirectly lost: 12,078 bytes in 210 blocks
==151909==      possibly lost: 2,216 bytes in 7 blocks
==151909==    still reachable: 1,231,962 bytes in 6,447 blocks
==151909==         suppressed: 0 bytes in 0 blocks
  • This Diff without the testcase:
==111248== LEAK SUMMARY:
==111248==    definitely lost: 108,461 bytes in 6,766 blocks
==111248==    indirectly lost: 14,074 bytes in 211 blocks
==111248==      possibly lost: 72 bytes in 2 blocks
==111248==    still reachable: 1,231,241 bytes in 6,446 blocks
==111248==         suppressed: 0 bytes in 0 blocks

And it is still statistically equal to the appresented at D11434 from master whitout uncoommenting the for at return_string_from_virtual and out_string_from_virtual methods at src/tests/efl_mono/Strings.cs test cases:

==112684== LEAK SUMMARY:
==112684==    definitely lost: 107,200 bytes in 6,691 blocks
==112684==    indirectly lost: 14,468 bytes in 218 blocks
==112684==      possibly lost: 0 bytes in 0 blocks
==112684==    still reachable: 1,188,177 bytes in 6,878 blocks
==112684==         suppressed: 0 bytes in 0 blocks

Diff Detail

Repository
rEFL core/efl
Branch
remove-if-constexpr (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16499
Build 11000: arc lint + arc unit
lucas created this revision.Apr 1 2020, 2:27 PM
lucas requested review of this revision.Apr 1 2020, 2:27 PM

I confirm this patch removes the C++17 warnings I reported, but I'll let somebody else review the code.

felipealmeida accepted this revision.Apr 13 2020, 9:11 AM
This revision is now accepted and ready to land.Apr 13 2020, 9:11 AM