Skip to content

[NFC][offloader] Mark run() definition static to match its declaration#1250

Merged
alsepkow merged 1 commit into
llvm:mainfrom
alsepkow:user/alsepkow/static_fixup
May 28, 2026
Merged

[NFC][offloader] Mark run() definition static to match its declaration#1250
alsepkow merged 1 commit into
llvm:mainfrom
alsepkow:user/alsepkow/static_fixup

Conversation

@alsepkow

@alsepkow alsepkow commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Minor nit: run() in tools/offloader/offloader.cpp is forward-declared static but the definition omits the specifier. Adding it matches the other two helpers in this file and the readability rationale in the LLVM Coding Standards. NFC.

Assisted by Claude Opus 4.7.

The forward declaration on line 81 already has internal linkage via
`static`. C++ rules preserve that linkage on the later definition,
but per the LLVM Coding Standards (`Restrict Visibility'' /
`Anonymous Namespaces'' sections) the `static` specifier should
also appear on the definition so readers can tell at a glance that
the function is translation-unit-local without having to find the
forward declaration. The two other helpers in this file (readFile and
matchesRegexIgnoreCase) already follow this convention.

See: https://llvm.org/docs/CodingStandards.html#restrict-visibility

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@alsepkow alsepkow marked this pull request as ready for review May 28, 2026 23:13
@alsepkow

Copy link
Copy Markdown
Collaborator Author

CI builds are failing. Need this fix

@bogner bogner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Arguably we should also move main to the end of the file - the forward declaration of run here is pretty silly. But in any case, this change as is is correct, so that can be some later cleanup.

@alsepkow alsepkow merged commit 94ee3cd into llvm:main May 28, 2026
11 of 23 checks passed
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