Remove grammar import mechanism, preserve external symbol mechanism#2
Conversation
Co-authored-by: godgnidoc <20113688+godgnidoc@users.noreply.github.com>
Co-authored-by: godgnidoc <20113688+godgnidoc@users.noreply.github.com>
Co-authored-by: godgnidoc <20113688+godgnidoc@users.noreply.github.com>
Co-authored-by: godgnidoc <20113688+godgnidoc@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR successfully separates the grammar import mechanism from the external symbol mechanism, removing the former while preserving the latter. The changes enable embedding other languages as external symbols in grammars without the complexity of importing individual lexical/syntactical units.
Key changes:
- Removed
usingstatements and relatedterm.using/ntrm.usingrules from grammar definition - Preserved
ntrm.externalfor loading external grammars as symbols - Added
ExternalLoadercallback type to support external symbol loading viasyntax.Import()
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
grammar/grammar.grammar |
Removed USING terminal and using/usings rules; simplified term to single pattern; kept ntrm.external for external symbols |
include/aliox/grammar.h |
Added ExternalLoader callback type and overloaded Compile() method to accept external loader |
src/aliox/grammar.cpp |
Implemented external symbol loading logic; simplified term/ntrm processing by removing import-related branches |
generated/include/grammar/syntax.h |
Updated hardcoded syntax to match simplified grammar; removed Using struct and related code; adjusted symbol/formula IDs |
include/alioth/println_compat.h |
Added compatibility shim for fmt::println on fmt < 10.0.0 |
src/alioth/syntax.cpp, src/alioth/parser.cpp, src/aliox/template.cpp, src/alioth-cli/skeleton.cpp, src/alioth-cli/parse.cpp |
Added println_compat.h include for fmt compatibility |
.gitignore |
Added _codeql_build_dir/ to ignored paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inline void println(FILE* f, const S& format, Args&&... args) { | ||
| print(f, fmt::runtime(format), std::forward<Args>(args)...); | ||
| std::fputc('\n', f); | ||
| } | ||
|
|
||
| template <typename S, typename... Args> | ||
| inline void println(const S& format, Args&&... args) { | ||
| print(fmt::runtime(format), std::forward<Args>(args)...); | ||
| std::putchar('\n'); | ||
| } |
There was a problem hiding this comment.
The compatibility implementation unconditionally wraps format strings with fmt::runtime(), which bypasses compile-time format string validation even for string literals. This should only be used for runtime format strings. Consider removing fmt::runtime() wrapper to preserve compile-time checking:
template <typename S, typename... Args>
inline void println(FILE* f, const S& format, Args&&... args) {
print(f, format, std::forward<Args>(args)...);
std::fputc('\n', f);
}
template <typename S, typename... Args>
inline void println(const S& format, Args&&... args) {
print(format, std::forward<Args>(args)...);
std::putchar('\n');
}
The previous work to remove the import mechanism inadvertently broke the external symbol mechanism. This PR cleanly separates the two:
usingstatements,term.using,ntrm.usingfor importing lexical/syntactical units from other grammar filesntrm.externalfor embedding other languages as symbols in the current grammarChanges
Grammar definition (
grammar/grammar.grammar)USINGterminal,using/usingsrules,term.using,ntrm.usingntrm.external -> ID@name ARROW STRING@grammar SEMICOLON;termto single pattern (wasterm.define/term.using)Grammar loader (
src/aliox/grammar.cpp,include/aliox/grammar.h)ExternalLoadercallback type for loading external grammarssyntax.Import()Hardcoded syntax (
generated/include/grammar/syntax.h)Build compatibility
println_compat.hfor fmt < 10.0.0 compatibilityWarning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
center2.conan.io/home/REDACTED/.local/bin/conan conan install /tmp/conanfile.txt --output-folder=/tmp/conan_deps --build=missing(dns block)esm.ubuntu.com/usr/lib/apt/methods/https /usr/lib/apt/methods/https(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.