Skip to content

cmake : enable warnings in llama#10474

Merged
ggerganov merged 7 commits intomasterfrom
gg/cmake-warnings
Nov 26, 2024
Merged

cmake : enable warnings in llama#10474
ggerganov merged 7 commits intomasterfrom
gg/cmake-warnings

Conversation

@ggerganov
Copy link
Member

@ggerganov ggerganov commented Nov 24, 2024

Enable more compile warnings in CMake builds.

danbev
danbev approved these changes Nov 24, 2024
View reviewed changes
Copy link
Member

@danbev danbev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these blocks be moved to the parent CMakeLists.txt to avoid the duplication?

@ggerganov
Copy link
Member Author

ggerganov commented Nov 24, 2024

Probably we can refactor this in a function call and put it in the cmake folder to avoid duplication. But I think it's better to not call it in the root CMakeLists.txt since it will also apply to 3rd party sources (if any) and it's better to have the 3rd party sources decide their compile flags.

@slaren
Copy link
Member

slaren commented Nov 24, 2024

Agree, let's not duplicate this code. Either a function of storing C/C++ flags in a common variable should do it.

@ggerganov ggerganov marked this pull request as draft November 24, 2024 20:35
@ggerganov ggerganov marked this pull request as ready for review November 25, 2024 20:11
@ggerganov ggerganov requested a review from slaren November 25, 2024 20:12
@github-actions github-actions bot added the build Compilation issues label Nov 25, 2024
slaren
slaren approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@slaren slaren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also make sense to rename get_flags to something like ggml_get_flags to make it clear where it comes from.

@ggerganov
Copy link
Member Author

ggerganov commented Nov 25, 2024

I didn't notice get_flags comes all the way from ggml. I added llama_get_flags to keep things separated. PTAnotherL

@slaren
Copy link
Member

slaren commented Nov 25, 2024

I think it would be ok to use ggml_get_flags in llama.cpp to avoid duplicating the code, keeping functions like this in sync is always a source of errors.

@github-actions github-actions bot added the Nvidia GPU Issues specific to Nvidia GPUs label Nov 25, 2024
@ggerganov ggerganov merged commit ab96610 into master Nov 26, 2024
@ggerganov ggerganov deleted the gg/cmake-warnings branch November 26, 2024 12:18
@github-actions github-actions bot added the ggml changes relating to the ggml tensor library for machine learning label Nov 26, 2024
@NeoZhangJianyu NeoZhangJianyu mentioned this pull request Nov 26, 2024
Closed
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Dec 20, 2024
@ggerganov @arthw
cmake : enable warnings in llama (ggml-org#10474)
e8a97f6 
* cmake : enable warnings in llama

ggml-ci

* cmake : add llama_get_flags and respect LLAMA_FATAL_WARNINGS

* cmake : get_flags -> ggml_get_flags

* speculative-simple : fix warnings

* cmake : reuse ggml_get_flags

ggml-ci

* speculative-simple : fix compile warning

ggml-ci
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danbev danbev danbev approved these changes

@slaren slaren slaren approved these changes

Assignees

No one assigned

Labels

build Compilation issues examples ggml changes relating to the ggml tensor library for machine learning Nvidia GPU Issues specific to Nvidia GPUs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ggerganov @slaren @danbev