Add metadata api and set llvm 20 as default#36
Add metadata api and set llvm 20 as default#36zhouguangyuan0718 wants to merge 1 commit intoxgo-dev:goplusfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on enhancing the LLVM bindings by introducing new metadata-related APIs and updating the default LLVM version to 20. These changes improve the flexibility of working with LLVM metadata and ensure the project leverages the latest LLVM features, while also updating the Go module's minimum Go version requirement. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new metadata APIs, upgrades the Go version to 1.22, and makes LLVM 20 the default. While the code changes for the new APIs and build configurations appear correct and consistent with the project's style, the PR description states that no tests were run. Given the changes to the build configuration and Go version, this is a critical risk. It is imperative to run the full test suite to validate these changes and prevent potential build failures or regressions before merging.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| @@ -1,4 +1,4 @@ | |||
| //go:build !byollvm && !llvm14 && !llvm15 && !llvm16 && !llvm17 && !llvm18 && !llvm20 && !llvm21 | |||
| //go:build !byollvm && llvm19 | |||
There was a problem hiding this comment.
Silent breaking change for existing LLVM 19 users
The old tag (!byollvm && !llvm14 && ... && !llvm18 && !llvm20 && !llvm21) made LLVM 19 the default — any build without an explicit version tag would select it. The new tag !byollvm && llvm19 requires an explicit opt-in.
Any CI pipeline or user who was building without a version tag (relying on LLVM 19 as the default) will silently switch to LLVM 20 after this change. There is no compile-time error; the wrong LLVM version is simply picked up transparently.
Consider adding a note in the PR description or README about this migration requirement.
| md.C = C.LLVMMDString2(c.C, cstr, C.unsigned(len(str))) | ||
| return | ||
| } | ||
| func (c Context) MetadataType() (t Type) { |
There was a problem hiding this comment.
Naming inconsistency with existing metadata API
The existing metadata functions in this file use the MD prefix: MDString, MDNode. The new function uses the full word MetadataType, breaking the established convention. Consider renaming to MDType for consistency.
MetadataAsValue is less clear-cut since the upstream C function is LLVMMetadataAsValue, but if MDType is preferred, MDAsValue or MDValue would follow the same pattern.
| md.C = C.LLVMConstantAsMetadata(v.C) | ||
| return | ||
| } | ||
| func (c Context) MetadataAsValue(md Metadata) (v Value) { |
There was a problem hiding this comment.
Potential crash on nil Metadata
LLVMMetadataAsValue in upstream LLVM does not null-guard its MD argument — unlike the custom wrappers in IRBindings.cpp (e.g., LLVMSetMetadata2) which explicitly check if (!MD). Calling this with a zero-valued Metadata{} passes NULL across the CGo boundary and causes an unrecoverable SIGSEGV inside LLVM's C++ runtime.
Consider adding a guard:
func (c Context) MetadataAsValue(md Metadata) (v Value) {
if md.C == nil {
return
}
v.C = C.LLVMMetadataAsValue(c.C, md.C)
return
}
| md.C = C.LLVMMDString2(c.C, cstr, C.unsigned(len(str))) | ||
| return | ||
| } | ||
| func (c Context) MetadataType() (t Type) { |
There was a problem hiding this comment.
Missing blank lines and doc comments
Both new functions are inserted without a preceding blank line, diverging from the style of every other function in this file. Per gofmt and Go conventions, top-level declarations should be separated by blank lines.
Additionally, both exported functions lack Go doc comments. Standard convention requires a comment beginning with the function name, e.g.:
// MetadataType returns the metadata type for the given context.
func (c Context) MetadataType() (t Type) { ... }Without these, go doc output will be empty for these new API entries.
|
Clean, well-scoped PR. The metadata API additions are minimal and follow existing patterns. The main concerns are: (1) the LLVM 19 → 20 default switch is a silent behavioral change for users building without explicit version tags — worth calling out in the description or docs; (2) the |
zhouguangyuan0718
force-pushed
the
goplus_21
branch
from
8eae608 to
3108ac2
Compare
2 participants
Summary:
Test: