Skip to content

Simplify "extern inlines" in bitreader and bitwriter - #905

Merged
ktmf01 merged 4 commits into
xiph:masterfrom
solbjorn:simplify-extern-inlines
Jul 18, 2026
Merged

Simplify "extern inlines" in bitreader and bitwriter#905
ktmf01 merged 4 commits into
xiph:masterfrom
solbjorn:simplify-extern-inlines

Conversation

@solbjorn

Copy link
Copy Markdown
Contributor

Extern inlines are not clearly defined by the C standard and are often interpreted differently depending in the compiler. These ones already involve workarounds for MSVC.
Clang under certain optimization options (LTO etc.) also doesn't intepret this correctly and doesn't emit standalone symbols for those at all, leading to linking errors.

Simplify these by defining plain static inlines to be used within the corresponding files and separate global symbols to be used externally. This doesn't make the codegen worse for bitreader and bitwriter, but guarantees that the global symbols will always be defined regardless of the compiler and compilation options.

Closes #895.

solbjorn and others added 2 commits May 31, 2026 19:45
Extern inlines are not clearly defined by the C standard and are
often interpreted differently depending in the compiler. These
ones already involve workarounds for MSVC.
Clang under certain optimization options (LTO etc.) also doesn't
intepret this correctly and doesn't emit standalone symbols for
those at all, leading to linking errors.

Simplify these by defining plain static inlines to be used within
the corresponding files and separate global symbols to be used
externally. This doesn't make the codegen worse for bitreader
and bitwriter, but guarantees that the global symbols will always
be defined regardless of the compiler and compilation options.

Closes #895.

Signed-off-by: Alexander Lobakin <alobakin@mailbox.org>
@ktmf01

ktmf01 commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

I wonder whether the inline keyword should be dropped entirely. I just did for the bitreader, as those functions were used only for debugging. I'll need to check whether it makes sense for the bitwriter too.

@ktmf01

ktmf01 commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

I just did some performance testing, and it didn't seem to have any effect really. I'd rather fix it this way. Any thoughts?

@ktmf01

ktmf01 commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

I see I missed this warning

bitwriter.c:335:49: warning: 'bitwriter_grow_' is static but used in inline function 'FLAC__bitwriter_write_raw_uint32_nocheck' which is not static
  335 |         if(bw->capacity <= bw->words + bits && !bitwriter_grow_(bw, bits))
      |

Need to check that out as well

sezero added a commit to sezero/flac that referenced this pull request Jul 18, 2026
Upstream pull request: xiph#905:

Extern inlines are not clearly defined by the C standard and are
often interpreted differently depending in the compiler. These
ones already involve workarounds for MSVC.
Clang under certain optimization options (LTO etc.) also doesn't
intepret this correctly and doesn't emit standalone symbols for
those at all, leading to linking errors.

Signed-off-by: Alexander Lobakin <alobakin@mailbox.org>
@solbjorn

Copy link
Copy Markdown
Contributor Author

I just did some performance testing, and it didn't seem to have any effect really. I'd rather fix it this way. Any thoughts?

Sure, go ahead!

My primary goal was just to remove "extern inlines" as their definition in the C spec is very ambigous and can lead to such problems. So I only manually made sure that inlines are still inlines, but their global copies are always emitted.

I realize that FLAC has a CI matrix and never had such issues as I had, but out of 70 external libs that I have (mixed C and C++), only FLAC didn't want to link. The compilation flags are a bit custom, but consistent across the whole project.

I also wanted to compare object code size before/after, but as my home project is for Windows, I'm unable to do that some easy way (only 3rd-party apps non-trivial to build). It's much easier on Linux for me (scripts/bloat-o-meter.pl from the kernel repo, which just uses nm).

@ktmf01
ktmf01 merged commit bba8f29 into xiph:master Jul 18, 2026
18 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.

Windows/clang-cl: error linking static FLAC.lib

2 participants