Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
66 changes: 64 additions & 2 deletions include/smallstring.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,52 @@
*/

#pragma once
// smallstring reads fmt textually, always -- header-only, module consumer, module interface alike. It
// never imports fmt, in any configuration.
//
// It can afford to be that blunt because fmt, if it is a module at all, has to be built with
// FMT_ATTACH_TO_GLOBAL_MODULE: seastar reaches fmt through <fmt/format.h> in ~21 of its headers and
// will keep doing so, so fmt's declarations must stay attached to the global module or nothing in the
// tree links. That macro is precisely what makes textual and imported fmt the *same* entities ("you
// can mix TUs with either importing or #including the {fmt} API" -- fmt's own words), so a textual
// read here meets an imported fmt anywhere else in the program.
//
// The converse does not hold, which is why there is no `import fmt;` branch to balance this one: the
// fmt::formatter<basic_small_string> specialisation below derives from fmt::formatter<std::string_view>,
// and through an import that base resolves to fmt's *primary* template -- "no member named 'parse'",
// deleted constructor. That is fmt's behaviour, not smallstring's (a TU that does nothing but
// `import fmt;` and name fmt::formatter<std::string_view> fails identically), and it is why every
// attempt to route this header's fmt through the module has been a bug.
#if defined(STDB_USE_FMT_MODULE) && !defined(FMT_ATTACH_TO_GLOBAL_MODULE)
#error "smallstring reads fmt textually and specialises fmt::formatter, so an fmt built as a C++20 module must be built with FMT_ATTACH_TO_GLOBAL_MODULE -- otherwise its declarations attach to module `fmt` and the textual ones here cannot match them."
#endif

// Owned by module `smallstring` when the consumer builds with modules (SMALLSTRING_USE_MODULE).
//
// Outside that module the include degrades to the import, so these declarations are not ALSO
// re-declared in the global module. A header is textual everywhere or module-owned everywhere, never
// both: mixing the two gives every type here two definitions, and nothing links.
//
// `import` is legal in a global module fragment, so a .cppm that reaches this header from its GMF is
// fine. What is NOT fine is reaching it from inside an `export { }` block for the first time -- put it
// in that module's GMF instead.
#if defined(SMALLSTRING_USE_MODULE) && !defined(SMALLSTRING_MODULE_INTERFACE)

// fmt as well, not just the import. This header has always made <fmt/format.h> visible to whoever
// includes it, and plenty of code leans on that -- regression/string_test.cc includes only
// smallstring.hpp and then calls fmt::format. The module cannot carry those declarations across: fmt
// sits in its global module fragment and a GMF is not re-exported, so `import smallstring;` alone would
// silently take fmt away from every consumer the moment SMALLSTRING_USE_MODULE is turned on.
//
// Keeping the include here costs nothing: it is the same <fmt/format.h> the interface unit read, and
// under FMT_ATTACH_TO_GLOBAL_MODULE (see the top of this file) the same entities either way.
#include <fmt/format.h>

import smallstring;
Comment on lines +45 to +57

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve fmt visibility in module-mode includes

When SMALLSTRING_USE_MODULE is enabled, including this public header expands only to import smallstring;, so it no longer provides the <fmt/format.h> declarations that existing users get from the textual header. The module interface includes fmt only in its global module fragment and does not re-export it, so code such as regression/string_test.cc that includes smallstring.hpp and then calls fmt::format will fail under the new module-mode path unless every caller adds its own fmt include/import.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, and reproduced — a TU that includes only smallstring.hpp and calls fmt::format fails under SMALLSTRING_USE_MODULE with clang's own missing '#include ".../fmt/format.h"'; 'format' must be declared before it is used. Fixed in the commit above: the module-mode branch now pulls fmt in exactly the way the textual branch does. It costs nothing, since fmt is textual on both sides of the module boundary (or a module on both sides, under STDB_USE_FMT_MODULE), so the declarations are the same entities either way.


#else

#ifndef SMALLSTRING_MODULE_INTERFACE

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep assert visible in the module interface

When building smallstring.cppm without SMALLSTRING_PRELUDE, this new guard suppresses the header's <cassert> include, but the body still defines the fallback Assert macro as assert(...) and expands it throughout the exported inline/template definitions. import std.compat does not import the assert macro, so the default module-interface build depends on an unrelated header having defined it and otherwise fails to compile. Please include <cassert> in the module interface/global fragment or special-case the fallback before exporting the header body.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, and it does not compile — reproduced before changing anything. Fixed in ddac83d.

Precompiling smallstring.cppm with SMALLSTRING_PRELUDE left undefined:

smallstring.hpp:602:17: error: use of undeclared identifier 'assert'
smallstring.hpp:1013:13: error: use of undeclared identifier 'assert'
smallstring.hpp:1017:13: error: use of undeclared identifier 'assert'
smallstring.hpp:1043:17: error: use of undeclared identifier 'assert'
...

one per Assert() call site, exactly as you describe — and import std.compat cannot cover for it, since assert is a macro and macros do not cross a module boundary. Only the prelude was hiding this: clapdb always points SMALLSTRING_PRELUDE at a header that defines Assert, so the fallback never fires there. Anyone building the module the way this file's own comment describes — "without it, smallstring.hpp falls back to plain assert() as before" — got a module that does not build.

On placement, I went with the global module fragment rather than the header. Of your two suggestions only that one works: on the module path the header body is read from inside an export { } block, where a first-time #include is not allowed — which is precisely why the include block is guarded out in the first place. So the fallback's dependency now sits in smallstring.cppm, next to the SMALLSTRING_PRELUDE hook it backs up. It costs importers nothing (a macro expanded while the interface is preprocessed reaches no one), and <cassert> is just <assert.h>, so it carries none of the libstdc++ declarations the neighbouring note warns about.

I also checked the fix restores working assertions rather than merely silencing the error, which a stub #define assert would also have done: a consumer built against a no-prelude BMI compiles, links and runs (hello len=5 fmt=[ hello]), and its object file carries a real __assert_fail reference. The prelude path and the STDB_USE_FMT_MODULE path both still build and pass their probes.

#include <sys/types.h>

#include <cassert>
Expand All @@ -31,11 +76,20 @@
#include <type_traits>
#include <utility>

// This is the branch that carries the body, so it is the one that *defines* the formatter
// specialisation -- the case the note at the top of this file is really about. Textual, always.
#include <fmt/format.h>

#endif // !SMALLSTRING_MODULE_INTERFACE

namespace small {
#ifndef Assert
#define Assert(condition, message) assert((condition) && (message))
#endif
namespace {
// Not an unnamed namespace: entities there have internal linkage, and a C++20 module interface
// cannot reference an internal-linkage entity from an exported inline function or template
// ("'kMinAlignSize' has internal linkage and cannot be referenced from an exported ...").
namespace detail {
inline constexpr uint64_t kMinAlignSize = 8; // 64 bits for modern cpu
/**
* @brief Aligns a value up to the next multiple of N
Expand All @@ -54,7 +108,10 @@ template <uint64_t N>
return (n + N - 1) & static_cast<uint64_t>(-N);
}

} // namespace
} // namespace detail

using detail::AlignUpTo;
using detail::kMinAlignSize;

/**
* @brief Storage strategy enumeration for small string optimization
Expand Down Expand Up @@ -5452,6 +5509,9 @@ template <typename Char,
struct fmt::formatter<small::basic_small_string<Char, Buffer, Core, Traits, Allocator, NullTerminated, Growth>>
: fmt::formatter<std::string_view>
{
// Delegate to the string_view formatter, which parses and applies the format spec. A hand-rolled
// parse() that merely skips to '}' stores no state, so width, alignment, fill and precision are
// silently discarded -- fmt::format("{:>5}", small_string("foo")) would give "foo", not " foo".
using fmt::formatter<std::string_view>::parse;

auto format(const small::basic_small_string<Char, Buffer, Core, Traits, Allocator, NullTerminated>& str,
Expand Down Expand Up @@ -5651,3 +5711,5 @@ struct hash<small::basic_small_string<Char, Buffer, Core, Traits, Allocator, Nul
};

} // namespace std

#endif // owned by module smallstring
56 changes: 56 additions & 0 deletions smallstring.cppm
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Module interface for smallstring.
//
// The header is the single source of truth; this unit only attaches its declarations to module
// `smallstring`. See the note at the top of include/smallstring.hpp for the ownership rule.
module;

// smallstring deliberately calls Assert(), not assert(), so the consumer can substitute its own
// assertion. Textually that was done by #define-ing Assert before including the header. Modules take
// that away: the header is an `import` at the consumer, and its macros are baked in *here*, when the
// interface is compiled. So the hook has to live here.
//
// Point SMALLSTRING_PRELUDE at a header that #define-s Assert (and includes whatever that needs).
// Without it, smallstring.hpp falls back to plain assert() as before.
#ifdef SMALLSTRING_PRELUDE
#include SMALLSTRING_PRELUDE
#endif

// And that fallback needs <cassert> -- which is why this include is here rather than in the header.
//
// smallstring.hpp does include <cassert>, but only on its textual path: the module-interface path skips
// the whole include block, and it has to, because it is read from inside an `export { }` block where a
// first-time #include is not allowed. Its `#ifndef Assert` fallback still expands to assert(), though,
// so with no SMALLSTRING_PRELUDE to define Assert the interface would not compile -- "use of undeclared
// identifier 'assert'", once per call site. `import std.compat` cannot rescue it either: assert is a
// macro, and macros do not cross a module boundary.
//
// So the fallback's dependency belongs here, next to the hook it backs. It is a macro, expanded while
// this unit is preprocessed, so nothing about it reaches importers; <cassert> is just <assert.h>, and
// carries none of the libstdc++ declarations the note below is about.
#include <cassert>

// std comes in as `import std.compat` below, NOT as textual libstdc++ headers here.
//
// Textual <stdexcept> in this fragment would bake libstdc++'s <string> declarations into the BMI as
// global-module entities. Any consumer that then reads <string> textually -- directly, or through
// <fmt/format.h> -- re-declares basic_string.tcc's explicit instantiations on top of them and clang
// rejects it: "explicit instantiation of 'getline' does not refer to a function template". It reaches
// consumers that never name smallstring, too, because a module that imports smallstring carries those
// declarations onward in its own BMI.
#include <sys/types.h>

// fmt is textual, here and everywhere else in smallstring -- the header we are about to read
// specialises fmt::formatter, and it is the one heavy header that has to stay in this fragment. The
// note at the top of include/smallstring.hpp explains why it is never an `import fmt;`, and asserts
// the FMT_ATTACH_TO_GLOBAL_MODULE that lets a textual read here meet an imported fmt elsewhere.
#include <fmt/format.h>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Honor STDB_USE_FMT_MODULE in the module interface

When STDB_USE_FMT_MODULE is enabled, the public shim in include/smallstring.hpp imports fmt as a module before importing smallstring, but this module interface still always pulls in the textual <fmt/format.h>. In that configuration the exported fmt::formatter<small::basic_small_string<...>> specialization is attached to the textual fmt::formatter, while consumers format through the imported fmt::formatter, so fmt::format can fail to see the specialization or hit duplicate fmt declarations. Please mirror the same STDB_USE_FMT_MODULE branch here when building the BMI.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Checked this by building it rather than reasoning about it, and the conclusion is mixed: the diagnosis is wrong for how fmt is actually built here, but it correctly identifies an unstated dependency, so I have written that dependency down and made it self-checking. Fixed in c55555f.

The described failure does not occur. I built fmt's module the way our deps build builds it (deps.cmake.in passes -DFMT_ATTACH_TO_GLOBAL_MODULE whenever STDB_USE_FMT_MODULE is on), built the smallstring BMI with STDB_USE_FMT_MODULE defined, and compiled a consumer that reaches smallstring through the public shim — so import fmt; followed by import smallstring;, precisely the configuration this comment is about — and called fmt::format on a small_string. It compiles, links and runs. fmt::format("[{:>8}]", s) gives [ hello], so the specialisation the consumer resolves to really is the module's (it inherits parse() from formatter<string_view>; a missed specialisation could not have applied the width).

The reason is FMT_ATTACH_TO_GLOBAL_MODULE: it detaches every fmt declaration from module fmt, so the imported fmt::formatter and the textual fmt::formatter are the same global-module entity. That is the macro's stated purpose — fmt's own comment is "you can mix TUs with either importing or #including the {fmt} API". There is no second fmt::formatter for the specialisation to attach to.

The suggested change does not compile, in either fmt flavour. The specialisation derives from fmt::formatter<std::string_view>, and through import fmt; that base resolves to fmt's primary template: no member named 'parse' in 'fmt::formatter<std::basic_string_view<char>>', deleted constructor. This is fmt's behaviour, not smallstring's — a TU containing nothing but import fmt; and a mention of fmt::formatter<std::string_view> fails the same way, while the identical TU with a textual <fmt/format.h> compiles. So the interface unit cannot mirror the shim's branch even if it wanted to.

What the comment is right about is the invariant. I rebuilt fmt's module without FMT_ATTACH_TO_GLOBAL_MODULE and the consumer does break — declaration 'basic_appender' attached to named module 'fmt' cannot be attached to other modules, an error pointing into fmt's headers from a TU that need not mention smallstring at all. That configuration is one this project cannot use anyway (without the macro, fmt's symbols get W3fmt module mangling and no textual includer — seastar, most of the tree — can link), but nothing said so. So smallstring.cppm now explains why the include is textual and what it depends on, and #errors if STDB_USE_FMT_MODULE is set while FMT_ATTACH_TO_GLOBAL_MODULE is not, so the failure is named at the point of cause instead of surfacing in someone else's TU. I also corrected the comment in smallstring.hpp, which claimed fmt was "a module on both sides" under STDB_USE_FMT_MODULE — it never was, and that wording is probably what prompted this review.

Default builds are unaffected (STDB_USE_FMT_MODULE is off, the #if is dead); I rebuilt the BMI and libsmallstring_module.a in build.ci to confirm.


export module smallstring;

import std.compat;

#define SMALLSTRING_MODULE_INTERFACE 1
export {
#include "smallstring.hpp"
}
#undef SMALLSTRING_MODULE_INTERFACE
Loading