[Cpp Parser] Implement free function declarations - #476
Conversation
f2da397 to
7a6290c
Compare
hoe-jo
left a comment
hoe-jo
left a comment
There was a problem hiding this comment.
maybe add tests for:
- overloads
- static/anon-ns, variadic ...,
- namespace-scope operator==
- extern "C", decl/def const mismatch
- multi-TU internal linkage
- ctor/dtor bodies
| if existing.source_location == Default::default() { | ||
| existing.source_location = incoming.source_location.clone(); | ||
| } | ||
|
|
||
| if existing.entity_type != incoming.entity_type { | ||
| warn!( | ||
| "conflicting entity types while merging '{}': keeping {:?}, dropping {:?}", | ||
| existing.id, existing.entity_type, incoming.entity_type | ||
| ); | ||
| } |
There was a problem hiding this comment.
merge keeps first-seen entity_type and source_location, so a forward decl in an earlier TU wins over the real definition.
There was a problem hiding this comment.
Yes, that's correct. At the moment, source_location is intentionally modeled as a single representative location for the entity, so the merge keeps the first-seen value rather than trying to retain every declaration/definition site. The downside is that, when a forward declaration is seen first, it can win over the full definition.
For example,
// a.h
namespace util {
class Widget;
}
// b.h
namespace util {
class Widget {
public:
int value;
};
}
If we see a.h first and b.h afterward, the merged entity may end up with the type-level source_location pointing to the forward declaration in a.h, while member-level source locations still point to their actual definitions in b.h.
There was a problem hiding this comment.
entity_type is different, since a later TU may refine the classification, and keeping the first-seen value can therefore be incorrect.
I will update the logic for entity_type
There was a problem hiding this comment.
source_location is pinned to the first-seen entity, so a forward declaration parsed first wins over the definition. namespace svc { class IThing; } in a.cpp, abstract IThing (virtual void doIt() = 0) in b.cpp → svc::IThing -> Class, loc fwd.hpp:2
| * SPDX-License-Identifier: Apache-2.0 | ||
| ********************************************************************************/ | ||
|
|
||
| #include "widget_forward.h" |
There was a problem hiding this comment.
Yes, this is intentional.
The purpose of this test case is to cover the scenario where the parser sees a forward declaration in one TU and the full definition in another, so first.cpp only includes the forward declaration on purpose.
| pub struct CallableArgumentIdentityKey { | ||
| pub param_type: Option<String>, | ||
| pub is_variadic: bool, | ||
| pub is_pack_expansion: bool, | ||
| } | ||
|
|
||
| impl From<&FunctionArgument> for CallableArgumentIdentityKey { | ||
| fn from(argument: &FunctionArgument) -> Self { | ||
| Self { | ||
| param_type: argument.param_type.clone(), | ||
| is_variadic: argument.is_variadic, |
There was a problem hiding this comment.
CallableArgumentIdentityKey.param_type uses Type::get_display_name(), not the canonical type.
Top-level const on a param is not part of a C++ signature, so void topconst(int); + void topconst(const int v) {} emits 2 declarations for 1 function
There was a problem hiding this comment.
Good catch. add logic for top-level const and volatile.
| }) | ||
| } | ||
|
|
||
| fn extract_free_function_declaration( |
There was a problem hiding this comment.
namespace_id() returns None for both global scope and anonymous namespaces, and seen_free_function_declarations spans all TUs — so static void f() / anon-ns f() in two different .cpp files collapse to one declaration while both functions entries survive
There was a problem hiding this comment.
update the logic with test internal_linkage_declaration_identity
| if !Self::insert_callable_identity( | ||
| seen_method_declarations, | ||
| CallableOwnerIdentityKey::Method { | ||
| class_id: class_id.clone(), | ||
| }, | ||
| &id.name, | ||
| ¶meters, | ||
| ) { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
extract_method_declaration inserts into seen_method_declarations before add_method_declaration runs, so when that bails with "incompletely registered owning class" the entry is burned
There was a problem hiding this comment.
Good catch. update the logic, only insert once add method declaration correctly.
|
7a6290c to
c912d2c
Compare
| fn render_resolved_type_for_signature_identity(resolved: &ResolvedType) -> String { | ||
| strip_top_level_cv_qualifiers_ref(resolved).render_for_display() | ||
| } |
There was a problem hiding this comment.
cv-stripping fixes only one of three C++ parameter adjustments:
using MyInt=int; void alias_fn(MyInt); + void alias_fn(int v){} → 2 declarations for one function. Same for typedef int OldInt, and for void arr_fn(int[5]) vs void arr_fn(int*) → 2 entries. ResolvedType retains typedef sugar and skips array→pointer decay
| if existing.source_location == Default::default() { | ||
| existing.source_location = incoming.source_location.clone(); | ||
| } | ||
|
|
||
| if existing.entity_type != incoming.entity_type { | ||
| warn!( | ||
| "conflicting entity types while merging '{}': keeping {:?}, dropping {:?}", | ||
| existing.id, existing.entity_type, incoming.entity_type | ||
| ); | ||
| } |
There was a problem hiding this comment.
source_location is pinned to the first-seen entity, so a forward declaration parsed first wins over the definition. namespace svc { class IThing; } in a.cpp, abstract IThing (virtual void doIt() = 0) in b.cpp → svc::IThing -> Class, loc fwd.hpp:2
c912d2c to
625e509
Compare
hoe-jo
left a comment
hoe-jo
left a comment
There was a problem hiding this comment.
Merge now, findings will be addressed in follow up PR
2 participants
No description provided.