Skip to content

Conversation

@jayrm
Copy link
Member

@jayrm jayrm commented Nov 7, 2018

While investigating the lbound & ubound bug, found an unrelated bug where internal mangling of the array descriptor was conflicting between const & non-const types

  • mangling: mangle top-level const in to internal array descriptor structs

jayrm added 2 commits November 6, 2018 19:29
- internal lbound() and ubound() run-time functions were being declared expecting non-const bydesc array() as any.
- added regression test
- reference: https://www.freebasic.net/forum/viewtopic.php?p=254260#p254260
aliasid &= dimensions
end if

'' top level const will be ignored in symbMangleType(), so use an alternate
Copy link
Member

@dkl dkl Nov 7, 2018

Choose a reason for hiding this comment

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

It seems wrong to duplicate the code from symbMangleType(), and to mangle a part of the arraydtype outside the <...> part intended for it. But using something like symbMangleType(typeSetIsRef(arraydtype)), like symbMangleParam(), would cause conflicts if FB ever supports arrays of references.

Maybe symbMangleType() needs an option to include the toplevel const for certain cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I'm going to give symbMangleType() an option parameter.

@jayrm
Copy link
Member Author

jayrm commented Nov 8, 2018

I added an option parameter to symbMangleType(). I decided to use an enum because it is convenient to search for the use of the option instead of "TRUE"; and in future allows other options to be added.

The top level const qualifier is now changed to be mangled as part of the array descriptor's data type. Should demangle correctly, (though demangling never works for me on win gdb).

I don't know much about how abbreviations work,; I copy/pasted the hAbbrevAdd() pattern.

symbGetDescTypeArrayDtype() was not returning the full data type, just DT and PTR. I actually expected that changing that would break things. Seems OK with current test suite. I didn't write any new tests for const arrays.

I will make the fix for UDT const array field access and const warnings in a separate PR.


'' const array?
if( typeIsConst( dtype ) ) then
if( (options and FB_MANGLEOPT_KEEPTOPCONST) <> 0 ) then
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the toplevel const checks in this function should both be moved between the ref and ptr checks, such that:

  • "reference to const" will only mangle the const once (avoiding KRK) even with FB_MANGLEOPT_KEEPTOPCONST
  • allow mangling "const foo const ptr" as KPK (probably not valid for C++?) by handling the toplevel const (dropping or mangling) before recursing in the ptr case

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, OK. I think you're correct.

Looks like KPK is valid for C++, as in this example will mangle the parameter to KPKi

template <class T>
T P (T a) {
 return a;
}

void proc()
{
	int const x = 5;
	int const* const y = &x;
	int const* const z = P <int const* const> (y);
}

c++filt will demangle KRKi to int const& const though looks like there's no legal way to ever use that in C++.

@jayrm jayrm merged commit b240a65 into freebasic:master Nov 11, 2018
@jayrm jayrm deleted the rtl-const branch November 11, 2018 03:32
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.

2 participants