<div dir="auto"><div><div class="gmail_quote"><div dir="ltr">Am Mo., 26. Nov. 2018, 10:46 hat Ryan Joseph <<a href="mailto:ryan@thealchemistguild.com">ryan@thealchemistguild.com</a>> geschrieben:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
> On Nov 26, 2018, at 12:09 AM, Sven Barth via fpc-pascal <<a href="mailto:fpc-pascal@lists.freepascal.org" target="_blank" rel="noreferrer">fpc-pascal@lists.freepascal.org</a>> wrote:<br>
> <br>
> - your pretty name is wrong; the pretty name for a specialization with constants should be "TSomeGeneric<TSomeType, 42>", not "TSomeGeneric<SomeUnit.TSomeType, System.LongInt#42>" as it would be now<br>
<br>
I’ll change the # prefix for const params but the unit prefix was not added by me. See the line:<br>
<br>
prettynamepart:=typeparam.resultdef.fullownerhierarchyname(true);<br>
<br>
in pgenutil.pas.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Yes, but that is only because the code currently only handles types. For constants the addition of the type is not required, only the constant value. </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> - remove those tgenericparasym and tgeneric_*_parasym types; use simply tconstsym instead of ttypesym for const parameters, other code will have to check that correctly<br>
<br>
Ok, I was able to remove those and replace with tconstsym. Much cleaner now. <br>
<br>
> - even though you'll remove is_const and const_type from ttypesym again due to the above it's wrong to use putsmallset/getsmallset for an enum; use putbyte/getbyte or one of the "larger" put*/get* functions depending on the number of entries contained in the enum (Note: not a runtime check, just pick the correct one at compile time)<br>
<br>
Are you sure those should be changed? There seems to be lots of assumptions made that the generic params are ttypesym and I’m not confident that mixing in tconstsyms will be safe. Having said that I never liked that I mixed in those const_* members into the class.<br>
<br>
Like you mention below if the const type was restricted in the definition then it would make sense to change these to tconstsym. I guess I need to try and see how much code this change blows up.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Those assumptions will "simply" have to be checked/fixed. In the end we'll have cleaner code and thus it will be worth it. And with the help of the testsuite you can avoid regressions. :) </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> - get rid of tscannerfile.parsing_generic_type; it's not only at the wrong location (the name "parsing_generic_type" should already tell you that it has no place inside the *scanner*), but it's also used to solve something in a completely wrong way: you must *not* use current_structdef for the generic parameters as the only valid parameter list inside parse_generic_specialization_types_internal *is* paradeflist and genericdef.genericparas. If there is still a problem then you need to handle that differently (Note: also don't Exit from parse_generic_specialization_type_internal, because the idea is that all incompatibilities are shown at once, so use Continue to check the next parameter)<br>
<br>
Yes, that was a hack to access current_structdef safely but I guess that’s not what I should be doing. Let me see if I can get that same information from the 2 params you mentioned.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Maybe you can show a test case that is failing? </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> - remove the check for m_objfpc; yes it won't work with inline specializations in mode Delphi for now, but it can still be used for specializations in type or var sections; also one idea for an improvement I have for parsing inline specializations in mode Delphi would be if the compiler knows that the symbol left of the "<" can only be a generic (cause let's be honest: in most code types aren't overloaded with variables/constants and more often than not types begin with "T", so they shouldn't conflict with the names of more local variables/constants/fields either)<br>
<br>
I got a crash in Delphi mode but I can change that to an error.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">For now we should reject an inline specialization (aka block_type = bt_general) in mode Delphi if the found generic contains any constant parameter (declarations of such generics should be allowed). Later on we can lift that restriction once I've implemented the checks I mentioned. </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> <br>
> Also thinking about the feature a bit it would be better to enforce the type of the constant during the generic's declaration. After all I can do different things with a string const, an ordinal const or a set const. So it would be better to use "const <NAME>: <TYPE>" where type can be any type that results in a tconstsym. After all the type of the constant inside the generic is usually fixed and those few cases were a variadic constant would be necessary could be handled by "TMyGeneric<T; const N: T>" (which would not work right now, but as we also need to support "TMyGeneric<T; S: SomeIntf<T>>" due to Delphi compatibility that would be handled by the same solution).<br>
<br>
> This way you can also get rid of the cconstundefined and you can create a correct tconstsym right of the bat.<br>
<br>
Sure but I need to see how many assumptions this undoes in other areas. I seem to remember having problems getting the const to fit in as it was and that’s when they were all the same class.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">As I said above that code will have to become aware of constants. Might lead to more crashes for you right now, but the end result would be cleaner/better. As a help you could compile the compiler with -CR which would ensure that typecasts of classes are always done with "as", thus triggering an exception at the correct location. </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> <br>
> And as I had written for the helper extension: add tests. A feature like this will need many tests (you can use "tgenconst" as test name prefix). Also don't forget to test this with generic routines.<br>
> <br>
> Don't forget to check the code formatting as well, I saw a few locations where you had spaces where they don't belong. ;)<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Regards, </div><div dir="auto">Sven </div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div></div>