PixarAnimationStudios / OpenSubdiv

An Open-Source subdivision surface library.
graphics.pixar.com/opensubdiv
Other
2.88k stars 558 forks source link

Add [[fallthrough]] attribute to case statements when appropriate. #1314

Open Cory-Kramer opened 1 year ago

Cory-Kramer commented 1 year ago

I added [[fallthrough]] attributes to case statements when fallthrough behavior was intended.
Fixes build warnings -Wimplicit-fallthrough.

opensubdiv/opensubdiv/bfr/hash.cpp:392:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  392 |     case 14:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:392:5: note: insert '[[fallthrough]];' to silence this warning
  392 |     case 14:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:392:5: note: insert 'break;' to avoid fall-through
  392 |     case 14:
      |     ^
      |     break; 
opensubdiv/opensubdiv/bfr/hash.cpp:395:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  395 |     case 13:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:395:5: note: insert '[[fallthrough]];' to silence this warning
  395 |     case 13:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:395:5: note: insert 'break;' to avoid fall-through
  395 |     case 13:
      |     ^
      |     break; 
opensubdiv/opensubdiv/bfr/hash.cpp:398:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  398 |     case 12:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:398:5: note: insert '[[fallthrough]];' to silence this warning
  398 |     case 12:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:398:5: note: insert 'break;' to avoid fall-through
  398 |     case 12:
      |     ^
      |     break; 
opensubdiv/opensubdiv/bfr/hash.cpp:405:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  405 |     case 10:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:405:5: note: insert '[[fallthrough]];' to silence this warning
  405 |     case 10:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:405:5: note: insert 'break;' to avoid fall-through
  405 |     case 10:
      |     ^
      |     break; 
opensubdiv/opensubdiv/bfr/hash.cpp:408:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  408 |     case 9:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:408:5: note: insert '[[fallthrough]];' to silence this warning
  408 |     case 9:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:408:5: note: insert 'break;' to avoid fall-through
  408 |     case 9:
      |     ^
      |     break; 
opensubdiv/opensubdiv/bfr/hash.cpp:411:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  411 |     case 8:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:411:5: note: insert '[[fallthrough]];' to silence this warning
  411 |     case 8:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:411:5: note: insert 'break;' to avoid fall-through
  411 |     case 8:
      |     ^
      |     break; 
opensubdiv/opensubdiv/bfr/hash.cpp:417:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  417 |     case 6:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:417:5: note: insert '[[fallthrough]];' to silence this warning
  417 |     case 6:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:417:5: note: insert 'break;' to avoid fall-through
  417 |     case 6:
      |     ^
      |     break; 
opensubdiv/opensubdiv/bfr/hash.cpp:420:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  420 |     case 5:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:420:5: note: insert '[[fallthrough]];' to silence this warning
  420 |     case 5:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:420:5: note: insert 'break;' to avoid fall-through
  420 |     case 5:
      |     ^
      |     break; 
opensubdiv/opensubdiv/bfr/hash.cpp:423:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  423 |     case 4:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:423:5: note: insert '[[fallthrough]];' to silence this warning
  423 |     case 4:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:423:5: note: insert 'break;' to avoid fall-through
  423 |     case 4:
      |     ^
      |     break; 
opensubdiv/opensubdiv/bfr/hash.cpp:429:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  429 |     case 2:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:429:5: note: insert '[[fallthrough]];' to silence this warning
  429 |     case 2:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:429:5: note: insert 'break;' to avoid fall-through
  429 |     case 2:
      |     ^
      |     break; 
opensubdiv/opensubdiv/bfr/hash.cpp:432:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  432 |     case 1:
      |     ^
opensubdiv/opensubdiv/bfr/hash.cpp:432:5: note: insert '[[fallthrough]];' to silence this warning
  432 |     case 1:
      |     ^
      |     [[fallthrough]]; 
opensubdiv/opensubdiv/bfr/hash.cpp:432:5: note: insert 'break;' to avoid fall-through
  432 |     case 1:
      |     ^
      |     break; 
11 errors generated.
davidgyu commented 1 year ago

Filed as internal issue #OSD-429

barfowl commented 1 year ago

My understanding is that [[fallthrough]] was introduced in C++17. At this point, OpenSubdiv does not use any features specific to C++17, so this change will make C++17 a new minimum requirement to build it.

@davidgyu, the USD repository contains similar code for its internal hashing. In the spirit of keeping Pixar's projects reasonably aligned, has the issue of how/when to suppress similar fall-through warnings been raised there?

davidgyu commented 1 year ago

That's right, we're not ready to make changes to the code that would require C++17.

This hasn't been addressed yet in the OpenUSD code base. There is work in progress to align with recent vfx platform versions which specify C++17 and when that work is complete we will consider incorporating C++17 specific features.