Skip to content
Snippets Groups Projects

vkd3d-shader: Minor fixes to clangd warnings (clangd).

Merged Francisco Casas requested to merge fcasas/vkd3d:clangd_fixes into master
2 unresolved threads

Some fixes to valid clangd warnings, since I am using it as linter.

Also, I am removing enum hlsl_error_level on 4/6, since it doesn't seem to be meant to be used anywhere after 3/6.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • Subject: [PATCH 1/6] vkd3d-shader/tpf: Remove unnecessary fallthrough (clangd).

      What is the issue here exactly? I don't necessarily have an issue with the change, but I wouldn't say it's an improvement purely on its own merits either.

       #include "hlsl.h"
      +#include "vkd3d_shader.h"
       #include <stdio.h>

      Is that needed? Doesn't hlsl.h pull in vkd3d_shader_private.h, which pulls in vkd3d_shader.h?

      Subject: [PATCH 4/6] vkd3d-shader/hlsl: Remove enum hlsl_error_level.

      We might as well merge that with the preceding patch that removes the last users of these.

    • Author Developer

      Subject: [PATCH 1/6] vkd3d-shader/tpf: Remove unnecessary fallthrough (clangd). What is the issue here exactly? I don't necessarily have an issue with the change, but I wouldn't say it's an improvement purely on its own merits either.

      Nothing really, besides that by default clangd emits a warning for all fall-throughs (that are not annotated in a rather specific way) so this warning shows in many places, but those in 1/5 and 5/5 can be seamlessly removed.

      I agree that it is not important at all to go hunting for them, it just seemed like a natural think to do. Anyways, I joined 1/5 and 5/5 so it makes less noise. I can also remove them altogether.

       #include "hlsl.h"
      +#include "vkd3d_shader.h"
       #include <stdio.h>

      Is that needed? Doesn't hlsl.h pull in vkd3d_shader_private.h, which pulls in vkd3d_shader.h?

      No, it is not needed. I may have added it by mistake.

      Subject: [PATCH 4/6] vkd3d-shader/hlsl: Remove enum hlsl_error_level.

      We might as well merge that with the preceding patch that removes the last users of these.

      Done!

      Edited by Francisco Casas
    • Nothing really, besides that by default clangd emits a warning for all fall-throughs (that are not annotated in a rather specific way) so this warning shows in many places, but those in 1/5 and 5/5 can be seamlessly removed.

      This happens also on macOS (where the compiler is based on clang), and is the reason why we have -Wno-implicit-fallthrough in build-mac for the CI. I would solve it properly rather than reorganize our code around that. It seems that both gcc and clang accept __attribute__((fallthrough)); as a fall through marker. So I would defined a macro called VKD3D_FALLTHROUGH that expands to that on gcc and clang and to nothing on other compilers. Then I'd remove -Wno-implicit-fallthrough from build-mac.

    • I should add, though, that both proposed changes to avoid case fallthrough mostly make sense to me in their own, as they make the code a tad more readable, so I'm accepting the change anyway. My suggestion still stands for the other fallthrough cases in the code (mostly ir.c, I think), but doesn't have to be in this MR.

    • Please register or sign in to reply
  • Francisco Casas added 4 commits

    added 4 commits

    • d5421bd6 - vkd3d-shader: Remove unnecessary fallthroughs (clangd).
    • 82f8a641 - vkd3d-shader/tpf: Avoid translations to D3DDECLUSAGE and back (clangd).
    • 485932cf - vkd3d-shader/hlsl: Remove enum hlsl_error_level (clangd).
    • 581bb12c - vkd3d-shader/d3dbc: Use D3DSIO_TEXKILL instead of VKD3D_SM1_OP_TEXKILL (clangd).

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

    • This happens also on macOS (where the compiler is based on clang), and is the reason why we have -Wno-implicit-fallthrough in build-mac for the CI. I would solve it properly rather than reorganize our code around that. It seems that both gcc and clang accept __attribute__((fallthrough)); as a fall through marker. So I would defined a macro called VKD3D_FALLTHROUGH that expands to that on gcc and clang and to nothing on other compilers. Then I'd remove -Wno-implicit-fallthrough from build-mac.

      So the history of -Wimplicit-fallthrough in vkd3d is that I was sceptical about introducing it in commit 8b49b6e0, but we only had a single function that needed adjustment, and the gcc mechanism for doing that was innocuous enough. I don't think that's true for the Clang mechanism, and adding a macro for it doesn't help much, in my opinion. My preferred way of addressing this issue would be to simply get rid of -Wimplicit-fallthrough from configure.ac.

      That said, patch 1/4 would be fine with me if we also moved the default case in spirv_compiler_create() to the bottom of the switch.

    • So the history of -Wimplicit-fallthrough in vkd3d is that I was sceptical about introducing it in commit 8b49b6e0, but we only had a single function that needed adjustment, and the gcc mechanism for doing that was innocuous enough. I don't think that's true for the Clang mechanism, and adding a macro for it doesn't help much, in my opinion. My preferred way of addressing this issue would be to simply get rid of -Wimplicit-fallthrough from configure.ac.

      I think -Wimplicit-fallback has some value, but I can live without it.

    • I think -Wimplicit-fallback has some value, but I can live without it.

      I'd rather keep it if possible. I'm fairly sure it's caught my errors before.

    • Please register or sign in to reply
  • Francisco Casas added 4 commits

    added 4 commits

    • 38f1102d - vkd3d-shader: Remove unnecessary fallthroughs (clangd).
    • 02ba54fc - vkd3d-shader/tpf: Avoid translations to D3DDECLUSAGE and back (clangd).
    • a52b6f9a - vkd3d-shader/hlsl: Remove enum hlsl_error_level (clangd).
    • 62dfd1ed - vkd3d-shader/d3dbc: Use D3DSIO_TEXKILL instead of VKD3D_SM1_OP_TEXKILL (clangd).

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 16 commits

    added 16 commits

    • 62dfd1ed...6034f4a9 - 12 commits from branch wine:master
    • f0da419a - vkd3d-shader: Remove unnecessary fallthroughs (clangd).
    • 432fa8fa - vkd3d-shader/tpf: Avoid translations to D3DDECLUSAGE and back (clangd).
    • 79608365 - vkd3d-shader/hlsl: Remove enum hlsl_error_level (clangd).
    • 38a73097 - vkd3d-shader/d3dbc: Use D3DSIO_TEXKILL instead of VKD3D_SM1_OP_TEXKILL (clangd).

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading