Am Mo., 26. Nov. 2018, 10:46 hat Ryan Joseph <r...@thealchemistguild.com> geschrieben:
> > > > On Nov 26, 2018, at 12:09 AM, Sven Barth via fpc-pascal < > fpc-pascal@lists.freepascal.org> wrote: > > > > - 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 > > I’ll change the # prefix for const params but the unit prefix was not > added by me. See the line: > > prettynamepart:=typeparam.resultdef.fullownerhierarchyname(true); > > in pgenutil.pas. > 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. > > - remove those tgenericparasym and tgeneric_*_parasym types; use simply > tconstsym instead of ttypesym for const parameters, other code will have to > check that correctly > > Ok, I was able to remove those and replace with tconstsym. Much cleaner > now. > > > - 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) > > 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. > > 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. > 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. :) > > - 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) > > 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. > Maybe you can show a test case that is failing? > > - 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) > > I got a crash in Delphi mode but I can change that to an error. > 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. > > > > 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). > > > This way you can also get rid of the cconstundefined and you can create > a correct tconstsym right of the bat. > > 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. > 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. > > > > 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. > > > > Don't forget to check the code formatting as well, I saw a few locations > where you had spaces where they don't belong. ;) > Regards, Sven >
_______________________________________________ fpc-pascal maillist - fpc-pascal@lists.freepascal.org http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal