Pull Requests

General discussion for topics related to the FreeBASIC project or its community.
Juergen Kuehlwein
Posts: 284
Joined: Mar 07, 2018 13:59
Location: Germany

Pull Requests

Post by Juergen Kuehlwein »

Jeff,

i need your help! As you have noticed, i have written a lot of code and made several pull requests recently. Don´t worry, i don´t expect you to comment and review all of this right now.

You requested for smaller PRs, i know, but ultimately i must be able to test all new things together. More than once i had to change or even re-write parts of it as development proceded. And i admit i mis-used these PRs a bit for testing the code in Linux.

You can devide all these PRs into three categories:

1.) make file functions work with wide strings in Windows
2.) string and arry extensions
3.) compiler pp changes and other fixes

#1 obviously must be thoroughly tested, i made a post here in the forum and supply executables for testing (with currently no reponse at all, alas)

#2 has been added to the RTL currently, i could make one or more separate libraries for this in order to support your goal of reducing what´s contained in the RTL.

#3 summarizes everything not feature specific, what´s necessary to make #2 possible regarding the compiler. The added code is no witchcraft or overly complicated in any way nor does it touch core features. In case of the new pp operators and the improved overload resolution, it´s copying and adding to what´s already there.

Namely these are PR #195, #191 and #189. For beeing able to carry on i´m asking for commenting and reviewing in order to merge these three PRs.

Currently USTRINGs can handle embedded nulls internally, the new string functions can do this as well. Existing functions like LEFT et al. currently cannot do that (because they weren´t written to do that). This would be a possible next step for making USTRINGs a real STRING counterpart.


JK
St_W
Posts: 1626
Joined: Feb 11, 2009 14:24
Location: Austria
Contact:

Re: Pull Requests

Post by St_W »

Juergen Kuehlwein wrote:[..]And i admit i mis-used these PRs a bit for testing the code in Linux.[..]
If you want Travis-CI builds you can get those easily and for free for your fbc fork. Just login/register on https://travis-ci.com with your GitHub account and enable automatic builds for your fbc repository. No further configuration needed (as the build instructions are within the travis file in the repository root).
Juergen Kuehlwein
Posts: 284
Joined: Mar 07, 2018 13:59
Location: Germany

Re: Pull Requests

Post by Juergen Kuehlwein »

Thanks, will do for future branches.
coderJeff
Site Admin
Posts: 4326
Joined: Nov 04, 2005 14:23
Location: Ontario, Canada
Contact:

Re: Pull Requests

Post by coderJeff »

Juergen Kuehlwein wrote:Jeff,

i need your help! As you have noticed, i have written a lot of code and made several pull requests recently. Don´t worry, i don´t expect you to comment and review all of this right now.

You requested for smaller PRs, i know, but ultimately i must be able to test all new things together. More than once i had to change or even re-write parts of it as development proceded. And i admit i mis-used these PRs a bit for testing the code in Linux.
I thank you for your patience. The smaller and more self contained a PR can be, the easier it is to work through the changes or improvements needed to merge.

I will tell you honestly that I have some difficultly supporting some of your proposed changes. However, I do understand the concept and end goal you are going for, so it's not like it is a total loss. For me personally it's a difficult proposition to reconcile and I often think about the topic How to add new features? (syntax, user API). I want users to be able to contribute, and indeed there are quite a few users creating pull requests. This is a good situation. Perhaps it is a personal attitude of mine that I need to modify to progress the project. Will keep you posted.

So far, I have only looked at PR #189, and it is not correct. Not usable. I will post details in the PR. However, I do think I have a solution. I wasn't really looking to get in to the namespace related bugs and issues yet, since there is a lot of other work already started and not complete.

I haven't really found a good method for prioritizing the work. Oldest to Newest? Easiest to hardest? And so on...
Juergen Kuehlwein
Posts: 284
Joined: Mar 07, 2018 13:59
Location: Germany

Re: Pull Requests

Post by Juergen Kuehlwein »

Jeff,

my priority would be #191 first, #195 second, #189 last. I need #191 for further development in general, #195 covers one aspect for string extensions, which must be solved sooner or later but can still wait a bit. #189 tries to fix an existing bug, but as there is a workaround, can wait too.

I didn´t go too deep in all this namespace stuff, but the problem here, as you can see from my comments, seems to be that, cSymbolType eats the namespace prefix. So afterwards, in case it wasn´t a type, the lexer position isn´t correct anymore. I simply restore the lexer context to what it was, before cSymbolType is called. According to my tests this seems to fix the issue and i didn´t discover any unwanted side effects so far.


JK
Juergen Kuehlwein
Posts: 284
Joined: Mar 07, 2018 13:59
Location: Germany

Re: Pull Requests

Post by Juergen Kuehlwein »

Happy new year everyone!


@Jeff

i don´t ask for new standards here, i´m asking for making new things possible. Accepting my proposed compiler changes (#PR 191) mostly adds new pp operators, which makes a macro driven syntax possible (as disussed here: viewtopic.php?f=17&t=27933).

This doesn´t mean that from now on everyone must use it, everyone can still do as he pleases! Every user still has a choice, if he wants to make use of it or not. As already explained i could then supply a macro interface/API wrapping around code in a namespace (and all of this placeed into an include file). So users preferring the namespace syntax, could do so without any problem. And those preferring the macro syntax could do so as well. I could add a #define for activating the macro syntax. So basically everything is inside an include file inclosed in a namespace as you requested. Activating the macro syntax would require something like "#define use_macro_syntax" or #define macro_api" before the "#include ..." statement.

These changes allow for expanding the existing syntax without changing the compiler and thus having to re-test each and everything everytime in the compiler. As said above, it allows for, it doesn´t make it a must. In my view it´s a great way of adding new things to the language, without touching the inner working of the compiler. Those working with the macro syntax will maybe have to deal more with possible naming conflicts - true. But all, who don´t like it, can still use the available namespace syntax.

JK
coderJeff
Site Admin
Posts: 4326
Joined: Nov 04, 2005 14:23
Location: Ontario, Canada
Contact:

Re: Pull Requests

Post by coderJeff »

hi JK,

I am currently working on your pr206 (POKE ANY to invoke memmove). I'm OK with the concept, not the implementation. I'm working on it because having a built-in way access low level memmove (and memcpy) is generally helpful. Based on many user codes seems like this capability is missing from fb. However, I'm also expanding on it to explore ideas for the namespace project and adding a ./inc/fbc-int/memory.bi to formalize the declaration of all the built-in low level memory functions, [c|re|de]allocate, clear, fb_memmove, and fb_memcpy. Plus adding tests to the test suite. This should happen soon and I will respond elsewhere.

For pr191 which adds pre processor features, I feel like you have invented a syntax for your array feature and then very narrowly solved for your needs. This seems to miss opportunities to for concepts and ideas from v1ctor's basic-macro branch and paul does's framework. I don't really want to discuss your array feature now because I have only briefly looked at your source and what I think about it wouldn't be fair.

So, I'll focus on the concepts of expanding the fbc pre-processor as a macro languge.

In v1ctor's work, in addition to the no-parens macro feature is the unique id push/pop mechanism. And very interesting to me is in paul doe's inc/fbfw/templates.bi are the patterns that can be constructed in the existing fbc pp now.

About the new operators:

Anyone that has written macros must come to the conclusion eventually that the '#' and '##' operators can become very difficult at times for the human reader to parse; sometimes for the compiler as well. Maybe there is a case for new specialized operators, but first I think it's work looking at it from a more general approach.

Suppose I have these PP functions so it is very clear the intent (setting arguments about the naming aside for now), and that there may or may not be case for usage in and/or out of a macro/define definition context:
__FB_PP_STRINGIZE( expr )
__FB_PP_JOIN( expr1, expr2 ) or maybe __FB_PP_CONCAT( expr1 [, exprn...] )

Then to expand on this idea, I think I would like to have the reverse operations as well:
__FB_PP_TOKENIZE( string ) -- which would take a string and convert back to tokens
__FB_PP_GETLEFTOF( expr, separator )
__FB_PP_GETRIGHTOF( expr, separator )

And to help with the problem of when a macro gets expanded:
__FB_PP_EVAL( expr ) or possibly __FB_PP_EXPAND( expr )
Which indicates that the current expansion should pause, 'expr' gets immediately expanded, and then current expansion continues.

This format of named functions lends itself to just about anything that returns strings or tokens
__FB_PP_UCASE( expr )
__FB_PP_LCASE( expr )
__FB_PP_TRIM( expr )
__FB_PP_LTRIM( expr )
... etc

About #REDEF:

Generally, I think this is going to cause trouble for users even if the compiler can easily parse it. Based on what I think is only use in array.bi is to '#redef TO ,' which leads me to believe that there is probably a more general way to look at this. Just like the parameters passed to #macro only make sense to that #macro, it would be nice to have something like __FB_PP_SET_LOCAL( symbol, expr ) where the definition only has scope within the expansion of that macro and is automatically released when the expansion of the macro completes.


About typeof()

Maybe ok to typeof() return a string. Though, might get easily confused for usage sometimes, and I'm not sure how it will play out if typeof() starts getting thrown around in macros and expression.
dim s as string
var x as typeof(s)
var x = typeof(s)

More explicitly could use:
var x = __FB_PP_STRINGIZE( typeof(expr) )

Either way, have to caution that string representation of typeof() can only return text of a compatible type, not necessarily the original type definition as written by the user. But that's a different problem, not directly related.
Juergen Kuehlwein
Posts: 284
Joined: Mar 07, 2018 13:59
Location: Germany

Re: Pull Requests

Post by Juergen Kuehlwein »

Hi Jeff,

thanks for taking care of PR206!
I feel like you have invented a syntax for your array feature and then very narrowly solved for your needs
Yes, i only added, what was necessary to get my code working. I expanded on, what was already there trying to following the existing naming.

"Array.bi" was written before you made the array descriptor available with fbcint. So there is optimizing potential. Basically i just made it work. This part will need a re-write as soon as it´s clear, what and how pp changes will finally be available in the compiler. The C part shouldn´t need much changes, except there are bugs i missed.

So, I'll focus on the concepts of expanding the fbc pre-processor as a macro languge.
Having some more possibilities and operators would be a great thing! I agree on the need for a different naming scheme, when there are more operators, as you propose. Maybe "#COUNT#", "#JOIN#", "#SPLIT#", "#LEFTOF", etc. the leading "#" indicating it´s a pp statement and the trailing "#" for making it an operator (vs. a regular pp statement).

Care must be taken for what i implemented as "#?", the first part (for implementing quirk words) must not expand to a literal string, because this leads to problems when resolving overloaded functions with string parameters. It could expand to a 64 bit number instead of 32 bit though, or maybe you have a better idea.


Depending on what new operators will be available the #REDEF feature could be dropped at all. At the time of writing it, i couldn´t find a better way for implementing "TO", you are right.
it would be nice to have something like __FB_PP_SET_LOCAL( symbol, expr ) where the definition only has scope within the expansion of that macro and is automatically released when the expansion of the macro completes.
I absolutly agree, but to me it seems this would require a lot of new code. I always tried to get the most out the existing logic without touching the more complex parts inside the compiler. If you think we should have such a feature, please do!

var x = __FB_PP_STRINGIZE( typeof(expr) )
I need a method of retrieving a variable´s type at run-time and it must allow for keeping different kinds of UDTs apart. So TYPEOF seemed to be a natural choice and required the least coding effort, because it already works as pp statement. Keeping the use cases (pp or regular code) somehow syntactically apart from each other, as you propose, keeps the code more readable and avoids ambiguities for both the compiler and the user - seems reasonable, i agree.


JK
Juergen Kuehlwein
Posts: 284
Joined: Mar 07, 2018 13:59
Location: Germany

Re: Pull Requests

Post by Juergen Kuehlwein »

Jeff,


as already mentioned i usually try to keep the necessary changes as minimal as possible, but you suggested a more generic approach. It´s amazimg what you can do by combining and nesting macros and the already available macro operators including ("#" and "##" type of operators). Especially with multiline macros and conditional clauses you can do quite a lot. Paul´s code in templates.bi is a good sample.

In my case wanting to build a syntactical front end, which must end in a sub or function, this is not enough. When there is no return value, it can end in a sub, so multiline macros are fine. But when a return value is needed, i cannot make use of multiline macros, because this will throw a compiler error: result = <multiline macro> doesn´t work. I could pass a result parameter as a last resort, but this i kind of ugly.

Taking a more generic perspective and looking at what i actually coded and all the things i already tried, i think the following set of additional operators will do (see below). i don´t think we need a whole new macro language, expanding a bit on the available possibilities would be enough.
  • remove parameter count check for variadic parameters
  • count parameters in a variadic parameter:
    p = 1,2,3 #%#p... -> 3
  • stringize uppercase:
    p = asdf ###asdf -> "ASDF" (need this for case-insensitive comparison)
  • join:
    p = b c d #&#a, b c d -> bcd (don´t for "x y z" or (a s d f))
  • separate:
    p = a b c d #/#a b c d -> a, b, c, d (don´t for "x y z", (a s d f))
  • left/right split "<case insensitive literal>" #<"..."# , #>"..."# (split a parameter into two parts at the given character(s))
    example:
    p = a(b(7)), #<"("#p -> a, (b(7))
    p = "ab(c">, #<"("#p -> , "ab(c"
    p = <empty>, #<"("#p -> <empty>
    p = a(b(7)), #>"("#p -> a(b, (7))
  • isequal "<case insensitive literal>" #?"..."# (test, if a parameter is equal to the given literal)
    example:
    p = Any, #?"any"#p -> 1
    p = "abc", #?"any"#p -> "abc"
    p = <else> #?"any"#p -> <else>
    p = <empty>, #?"any"#p -> <empty>
With this set of operators, you can do almost everything i can think of. #REDEF could be dropped. Should i try to code this, or would you like to proceed as outlined in your last post?


JK
fxm
Moderator
Posts: 12107
Joined: Apr 22, 2009 12:46
Location: Paris suburbs, FRANCE

Re: Pull Requests

Post by fxm »

Difference of declaration between clear/fb_memmove/fb_memcopy and FBC.clear/FBC.memmove/FBC.memcopy

- clear/fb_memmove/fb_memcopy are declared to take 'byref as any' parameters for dst/src.
- FBC.clear/FBC.memmove/FBC.memcopy are declared to take 'byval as any ptr' parameters for dst/src.

But a common calling syntax can match with both declaration types by using an argument syntax as:
'byval @dst', 'byval @src'
(byval is taken into account when passing a pointer to a by-reference parameter, and byval is ignored when passing an argument to a by-value parameter)
Last edited by fxm on Jan 14, 2020 6:14, edited 1 time in total.
fxm
Moderator
Posts: 12107
Joined: Apr 22, 2009 12:46
Location: Paris suburbs, FRANCE

Re: Pull Requests

Post by fxm »

More annoying between Poke Datatype and Poke Any

Similar difference of declaration between 'poke datatype' and 'poke any', but here, no common syntax exists because passing 'byval @dst' is refused by 'poke datatype'.

Note:
'Poke Any' no longer works when 'fbc-int/memory.bi' is included (because 'fb_MemMove' is no longer defined but 'FBC.MemMove' instead)
Last edited by fxm on Jan 13, 2020 15:30, edited 1 time in total.
fxm
Moderator
Posts: 12107
Joined: Apr 22, 2009 12:46
Location: Paris suburbs, FRANCE

Re: Pull Requests

Post by fxm »

A question I ask myself for the follow up to give to these additions:
- fbc: add builtin function fb_MemMove() alias "memmove"
- fbc: add builtin function fb_MemCopy() alias "memcpy" (was previously removed in an older version of fbc)
- fbc: 'POKE ANY, dst, src, count' statement
- ./inc/fbc-int/memory.bi - fbc API for low level memory operations allocate, callocate, reallocate, deallocate, clear, memcopy, memmove
Should we / how will we enter all these new features in the documentation?
coderJeff
Site Admin
Posts: 4326
Joined: Nov 04, 2005 14:23
Location: Ontario, Canada
Contact:

Re: Pull Requests

Post by coderJeff »

fxm, thanks for the breakdown.

'fb_MemCopy' & 'fb_MemMove' should get their own wiki pages with descriptions and examples. Goal of 'fb_MemCopy' & 'fb_MemMove' is to have a general purpose 'copy-memory' and 'move-memory' statement be available in fbc. These low level bulk memory copy/move statements are intended to complement other low level memory functions like allocate & deallocate, independent of other includes like crt or winapi with similar purposed functions. The 'fb_MemCopy' & 'fb_MemMove' source & destination parameters follow the same call semantics as the destination parameter in 'Clear' statement. Addition of these new symbols in the global namespace should not affect user code since users should generally avoid "fb_*" prefixed symbols because the global namespace is full of "fb_*" symbols already. 'fb_MemCopy' used to exist in an old version of fbc but was likely removed as it was not referenced in fbc source, test suite, wiki. Only reference was (still is, unchanged) in one example program, './examples/xml/expat.bas'.

The POKE ANY syntax, I don't know, I could take it or leave it. I have no particular interest in developing it further. And if it is annoying or inconsistent with already existing POKE syntax, then it should just be removed, and instead prefer fb_MemMove(). IMHO, there is little benefit having POKE ANY quirk as an alias to invoke fb_MemMove().

The './fbc-int/memory.bi' include has some experimental purposes and I'll explain in next post.
coderJeff
Site Admin
Posts: 4326
Joined: Nov 04, 2005 14:23
Location: Ontario, Canada
Contact:

Re: Pull Requests

Post by coderJeff »

fxm, about './fbc-int/memory.bi': for now, documentation should probably only exist in developer pages. There's a couple of experiments here. In hindsight, I should have added a big EXPERIMENTAL tag to the include file from the beginning (can still do that).

Based on your break down, there's 2 things I could see doing to './fbc-int/memory.bi'
a) don't #undef anything, just re-declare in the fbc namespace. Let user decide to #undef
b) duplicate the normal fbc declarations (i.e. byref as any).

But before make any decision: I hope that the './fbc-int/*.bi' files can be a way to share fbc internals with users & casual developers. I also hope that we are not required to support this exact API forever, as that could impede development. So I will say this, './fbc-int/*.bi' files will be accurate for the current version of fbc. Users can make use of these includes if they choose. And developers reserve the right to change at any time. Internals may change as needed for development. I think that's fair.

1) A while back you asked me about wiki location for the fbc.FBARRAY structure. I didn't really have a good answer or reason that it should only be in developer documentation, or that it should be added to user documentation. We don't really have any rules for what should be considered developer only content. Anyway, you already did the work, so it's OK with me that it is featured in user documentation. If ever find that it causes too much confusion for users, just move it back to developer documentation.

2) The namespace project. One of the ideas I was exploring with './fbc-int/memory.bi' was moving all the symbols in this group out of the global namespace (the reason for the #undef's), and getting the symbols in to 'fbc' namespace. Turns out, can't work for all symbols in this group. In current fbc compiler 'allocate' and 'deallocate' must be defined globally for new/new[]/delete/delete[] operators to work. And as you found, caused issue for POKE ANY.

3) The real low level declarations. In fbc currently, the low level memory functions are just aliases for malloc, calloc, realloc, free, memset, memcpy, & memmove. And fbc normally presents a mixture of byval as any ptr & byref as any declarations to the user (what is documented in wiki). In './fbc-int/memory.bi' I tried declaring only the byval as any ptr variants as that most closely matches the actual low level function calls. The experiment here is what is most meaningful in trying to describe (declare in source) fbc's internals.

I suspect that './fbc-int/memory.bi' will not be directly useful to most users since the functions are all built-in to fbc already. However, it does help serve the purpose of documenting fbc's internals and I have written tests in the test-suite to help detect if/when we ever break usage / availability.
fxm
Moderator
Posts: 12107
Joined: Apr 22, 2009 12:46
Location: Paris suburbs, FRANCE

Re: Pull Requests

Post by fxm »

coderJeff wrote: The POKE ANY syntax, I don't know, I could take it or leave it. I have no particular interest in developing it further. And if it is annoying or inconsistent with already existing POKE syntax, then it should just be removed, and instead prefer fb_MemMove(). IMHO, there is little benefit having POKE ANY quirk as an alias to invoke fb_MemMove().
coderJeff wrote: ..... './fbc-int/memory.bi' ..... caused issue for POKE ANY.
Me too, I would not keep POKE ANY.

But if we ever keep it, I wish it had the same type of declaration as the current POKE:
declare sub poke any ( byval pdst as any ptr, byval psrc as any ptr, byval bytes as uinteger )
Consequently 'poke any, pdst, psrc, bytes' would act as 'fb_memmove(byval pdst, byval psrc, bytes)'.
Post Reply