Skip to content

[cmake] Use source_group TREE on source_group_by_folder #2019

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.

Sign up for GitHub

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

Merged
CastagnaIT merged 1 commit into from
Apr 20, 2026
+7 −2
Merged

[cmake] Use source_group TREE on source_group_by_folder #2019

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions Helpers.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,27 @@ function(source_group_by_folder target)
endif()
endif()
foreach(file ${files})
if(file MATCHES "^\\$<")
list(POP_FRONT files)
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 20, 2026

Choose a reason for hiding this comment

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

list(POP_FRONT files) only works because $< (ASCII 36) sorts before letters, putting gen-exprs at the front of the sorted list. It's fragile — any filename starting with a char < ASCII 36 (!, #, %, etc.) would sort first, and POP_FRONT would then remove a legitimate source file instead.

Filter gen-exprs from files before the loop and drop this line entirely:

Suggested change
list(POP_FRONT files)
continue()

Add before the foreach:

list(FILTER files EXCLUDE REGEX "^\\$<")

continue() alone is sufficient since the gen-expr is never appended to files when we skip the rest of the loop body.

Copy link
Copy Markdown

@kodiai kodiai Bot Apr 20, 2026

Choose a reason for hiding this comment

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

[MAJOR] Incorrect list manipulation during iteration.

The list(POP_FRONT files) command modifies the files variable while the foreach(file ${files}) loop is iterating over it. While foreach iterates over a snapshot (so this doesn't break the iteration), this line is problematic because:

  1. It has no effect on which files are processed (the loop continues with all files)
  2. The files variable is reused as an accumulator later in the loop (line 44 clears it, line 46 appends to it)
  3. If a generator expression appears after the first directory group is processed, this would pop from the accumulator instead of the original list, corrupting the results
  4. The continue() statement alone is sufficient to skip the generator expression
Suggested change
list(POP_FRONT files)
if(file MATCHES "^\\$<")
continue()
endif()

continue()
endif()

if(NOT IS_ABSOLUTE ${file})
set(file ${CMAKE_CURRENT_SOURCE_DIR}/${file})
endif()
file(RELATIVE_PATH relative_file ${relative_dir} ${file})
get_filename_component(dir "${relative_file}" DIRECTORY)
if(NOT dir STREQUAL "${last_dir}")
if(files)
source_group("${last_dir}" FILES ${files})
source_group(TREE "${relative_dir}" FILES ${files})
Copy link
Copy Markdown

@kodiai kodiai Bot Apr 20, 2026

Choose a reason for hiding this comment

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

[MAJOR] Non-standard usage pattern for source_group(TREE ...).

The source_group(TREE <root> FILES ...) command is designed to be called once with all source files to automatically create the complete folder hierarchy. The current implementation calls it multiple times with directory-grouped subsets (triggered by the if(NOT dir STREQUAL "${last_dir}") condition at line 40).

While multiple calls may work, this approach is:

  • Unconventional and more complex than necessary
  • Inconsistent with CMake documentation examples
  • Potentially fragile if CMake's behavior changes

Standard pattern:

# Filter generator expressions
set(filtered_files "")
foreach(file ${files})
  if(NOT file MATCHES "^\\$<")
    list(APPEND filtered_files "${file}")
  endif()
endforeach()

# Create hierarchy in one call
if(filtered_files)
  source_group(TREE "${relative_dir}" FILES ${filtered_files})
endif()

This would eliminate the entire directory-grouping loop (lines 29-48) since TREE handles the hierarchy automatically.

endif()
set(files "")
endif()
set(files ${files} ${file})
set(last_dir "${dir}")
endforeach(file)
if(files)
source_group("${last_dir}" FILES ${files})
source_group(TREE "${relative_dir}" FILES ${files})
endif()
endfunction()

Expand Down
Loading
⚡