Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix clang 19 warnings/errors #522

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

nmschulte
Copy link
Contributor

Using clangd/clang 19 and Neovim LSP client/server, these changes were warnings (or sometimes errors).

controllers/actuators/electronic_throttle_impl.h|13 col 1-20 warning| Included header sensor.h is not used directly (fix available)

controllers/actuators/electronic_throttle_impl.h|81 col 2-12 error| Unknown type name 'SensorType' (fix available)

controllers/actuators/electronic_throttle_impl.h|94 col 2-7 error| Unknown type name 'Timer' (fix available)
controllers/algo/rusefi_hw_enums.h|23 col 19-27 error| Unknown type name 'uint16_t' (fixes available)
util/efilib.h|105 col 35-39 error| Unknown type name 'Gpio' (fix available)
util/efilib.h|110 col 10-14 error| Use of undeclared identifier 'Gpio' (fix available)
init/sensor/init_lambda.cpp|6 col 1-23 warning| Included header live_data.h is not used directly (fix available)
controllers/math/engine_math.cpp|24 col 1-28 warning| Included header event_registry.h is not used directly (fixes available)
controllers/math/engine_math.cpp|25 col 1-23 warning| Included header fuel_math.h is not used directly (fixes available)
controllers/algo/fuel_math.cpp|33 col 1-27 warning| Included header speed_density.h is not used directly (fix available)
init/sensor/init_tps.cpp|9 col 1-35 warning| Included header auto_generated_sensor.h is not used directly (fix available)
controllers/engine_cycle/knock_logic.h|15 col 36-48 error| Expected class name (fix available)

controllers/engine_cycle/knock_logic.h|35 col 31-36 error| Use of undeclared identifier MS2NT (fix available)
controllers/sensors/impl/software_knock.cpp|98 col 33-50 warning| Result of integer division used in a floating point context; possible loss of precision
controllers/system/thread_controller.h|18 col 33-43 error| Use of undeclared identifier 'chibios_rt' (fix available)
controllers/system/thread_controller.h|18 col 45-61 error| Unknown template name 'BaseStaticThread'
controllers/system/thread_controller.h|21 col 8-15 error| Unknown type name 'tprio_t' (fix available)
controllers/system/thread_controller.h|28 col 14-22 error| Only virtual member functions can be marked 'override' (fix available)
controllers/system/thread_controller.h|29 col 9-16 error| No member named 'setName' in 'ThreadController<TStackSize>'
controllers/system/thread_controller.h|37 col 37-44 error| Unknown type name 'tprio_t' (fix available)
controllers/system/thread_controller.h|51 col 4-14 error| Use of undeclared identifier 'chibios_rt' (fix available)
controllers/system/thread_controller.h|51 col 44-46 error| No member named 'start' in the global namespace; did you mean simply 'start'? (fix available)
@rusefillc
Copy link
Contributor

@nmschulte is there/should be there a CI configuration showing these warnings/errors?

@nmschulte
Copy link
Contributor Author

nmschulte commented Dec 10, 2024

@nmschulte is there/should be there a CI configuration showing these warnings/errors?

That is an interesting idea, but it would require running clang/clangd vs gcc. I can look into it if it's something we're interested in. To dial in my clangd configuration I learned about clangd --check=FILE which runs clangd for just one file.

https://clangd.llvm.org/guides/include-cleaner seems to be the feature that is telling about these #includes; I don't think GCC provides a similar mechanism.

Managing #includes

clangd has features to help you maintain the set of #include directives at the top of each file that are used to import dependencies.

It follows the include-what-you-use model: each source file should #include headers that declare the symbols it references, and no others.

This means:

  • files should not rely on transitive includes, only headers they include directly
  • #include directives describe direct dependencies between files, and it’s possible to reason about them locally

This is opinionated, and stricter than “will it compile”. It implies adding #includes that the compiler does not strictly need. Some #includes that satisfy the compiler should be replaced with more specific ones. If your codebase broadly aims to follow this style, you should enable these features.

Worth reiterating: This is opinionated, though that isn't necessarily a bad thing: it's helpful.

@rusefillc
Copy link
Contributor

@nmschulte does not macos build run clang by default/always?

more or less my question is: why change anything, what's the problem statement?

@nmschulte
Copy link
Contributor Author

I don't believe MacOS uses clang; I believe it uses Arm's GCC for that platform.

I suppose in essence, what this PR is proposing is: https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md

I wasn't quite aware of that when I created this PR, but I think it's worth considering.

@rusefillc
Copy link
Contributor

@nmschulte -Xiwyu: Tells GCC to use the IWYU plugin.

@nmschulte
Copy link
Contributor Author

-Xiwyu: Tells GCC to use the IWYU plugin.

@rusefillc where's that reference from?

@rusefillc
Copy link
Contributor

@nmschulte google AI could be hallucinating?

image

@mck1117
Copy link
Collaborator

mck1117 commented Dec 10, 2024

I don't believe MacOS uses clang; I believe it uses Arm's GCC for that platform.

macos uses clang for unit tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants