Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding #embed #810

Draft
wants to merge 41 commits into
base: master
Choose a base branch
from
Draft

Adding #embed #810

wants to merge 41 commits into from

Conversation

chuggafan
Copy link
Contributor

This does NOT include features such as the prefix and postfix or limit, which are needed for standards compliance.

This draft PR is KNOWN to not build, half of this PR's purpose is allowing me to show a proposal for the design of the internal hook mechanism of the compiler so that we're not wasting cycles making the compiler recompute known values.

The other half is that it seems like the compiler chokes on #809 and this needs to be investigated separately, for the immediate future it seems like I can just do a boolean check instead of comparing against nullptr.

Furthermore, I added a commit that temporarily alleviate #809 ( 30673702e59b78222fef71d4986d2bdab260cf44 ) however that has a different access violation with a nullptr'd std::list.

It's been awhile so I will try debugging these but I'm dropping this here as I have no idea the source for the latter and the former should 100% be fixed, I don't know what I did with this file but it just breaks things.

@LADSoft
Copy link
Owner

LADSoft commented Jun 30, 2023

yeah hooking the preprocessor that way is what is intended...

im glad you are working on this, I've been thinking maybe you are right, and the language specifications for C23 and C++17 are low-hanging fruit that should be addressed sooner rather than later...

i rewrote the VS projects last night so you might have to redo a little work...

@chuggafan
Copy link
Contributor Author

yeah hooking the preprocessor that way is what is intended...

Moreso I'm interested in the hook between the preprocessor back to the declaration engine, since in order to make this fast we need to actually be able to insert the values into the declaration during compile, otherwise we're doing a lot of extra tokenization for little-to-no value.

In terms of effort this amounts of a minor rewrite of the declaration engine in its entirety as far as I'm aware to deal with this fact.

@LADSoft
Copy link
Owner

LADSoft commented Jun 30, 2023

you wouldn't have to modify the entire declaration sequence, just the initialization aggregrator.

I would be tempted to use some kind of trick, like when intialize finds a structure/array to initialize it saves the current symbol somewhere, then when #embed is encountered it calls some callback that fills in the data for the variable (and maybe sets its size if it is an array????) and finally when it is all done a 'special' token is put into the input stream which tells initialize_aggregate_type not to do any processing... that would also scale well if you have a structure where you want to use #embed for one part of the structure but want to fill in things in code for another part of it... but i guess there would have to be a check somewhere if you are initializing a structure or sized array that would that you don't overrun the end...

@chuggafan
Copy link
Contributor Author

I would be tempted to use some kind of trick, like when intialize finds a structure/array to initialize it saves the current symbol somewhere,

My thoughts were that during the declaration/initialization sequence after the declarator is found we set the embed_function callback I made with the current information required from the lexer captured as part of it (since std::function can allow for that) for the declaration then continue during the initialization sequence we just append new bytes as needed, so that constructs such as:

char thingy[] =  {
0, 1, 2, 3,
#embed <file> limit(12) prefix(4, 5, 6) postfix(19, 20, 21)
, 22, 23, 24 // (note the first comma)
};

Work correctly. I may not actually do this portion on this PR but I will definitely be thinking about it during it...

@LADSoft
Copy link
Owner

LADSoft commented Jun 30, 2023

that would work; you might further be able to further speed it up by making some class 'Embedded Data' which has an accessor to return the next byte of data... then make a new lexeme type that would take a pointer to that class as data. Then in expression_primary when you run into it you get the next byte of data and make an 'expression', 'type' pair for it which you return as normal.

That way the rest of the code for parsing initializers remains completely unchanged. You wouldn't call getsym() until you ran out of data so it would keep processing the same lexeme token over and over until it was done... All that would get around having to build a ton of lexeme structures to reflecting the incoming data...

Copy link
Owner

Choose a reason for hiding this comment

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

hi chuggafan! This is looking good. But the way the project is now organized, occparse.exe depends on ocpp.lib... so files in ocpp.lib don't need to be added in occparse.lib they will just automatically be propagated.

Copy link
Owner

Choose a reason for hiding this comment

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

it was a good catch, that 'embed' should only be available in the compiler. not the assembler. For your reference ocpplib could be used elsewhere though, for example it is used in the rc compiler. I think that will probably fit in with the rest of your design though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I forget about orc half the time simply because I've never used windows resources (I have, sadly used COM apis though)

@LADSoft
Copy link
Owner

LADSoft commented Sep 17, 2023

@chuggafan, I finally read the docs for this, and I was thinking, we might have an extension that changes word size and endianness... that could be added later though. Anyway I'm mostly through with the C23 stuff although haven't pushed the latest yet... that leaves this as pretty much the only outstanding issue for C23. I was wondering what the status is? Will you have time to work at it sometime soon? I can work with you on the interface to the compiler if that is worrying you...

@chuggafan
Copy link
Contributor Author

I was working on the interface change, but between vacation + other things (and the fact that the change is just so large) I have been on a semi-pause, I have uploaded my current work so you can see what I'm doing, if you want me to pursue that method still instead of going after another in a bit, I'll continue working on it in the background, here's the current work, it doesn't compile at all (in fact just compiling for the preprocessor gives me 302 errors), but it still requires some extraneous work (e.g. substituting the static IsSymbolChar in main() for the proper one, giving PreProcessor a default (which will happen last), etc.)

@LADSoft
Copy link
Owner

LADSoft commented Sep 18, 2023

i took a quick look and what you are doing seems reasonable. But we are possibly going to have to update the compiler for those function-pointer-as-template argument things you've got going, I don't know what it will do with them.

We aren't in any hurry on finishing up the 'embed' tag I guess, so you can keep going along these lines if you want.

@chuggafan
Copy link
Contributor Author

I'm currently not done with the work on this draft pr, as it's SIGNIFICANTLY ballooned in scope (the change from cpp files to .h files for a number of things including kw enums for stuff like the preprocessor basically means I have to namespace large sections of code in order to ensure that it's correct).

I am also debugging some random MS issues with my code that aren't exactly clear in their error reports, so I am also trying to diagnose that (what fun).

In the meantime, this PR may die for quite some time and I might even switch to trying to send patches another way because github is requiring 2fa via some phone or secret app for my account, and because this account is tied with my "public" identity rather than my "private" (offline) identity I am not putting a phone number within lightyears of this account. So this may languish for quite some time if I am no longer able to do things such as make commits as of tomorrow. I will try to upload all of my current work tonight, however.

@LADSoft
Copy link
Owner

LADSoft commented Oct 5, 2023

my interpretation is if I didn't do the 2fa by the deadline, I would lose access to my github account going forward.... after the deadline there is no longer a way to log into the account if you don't have 2fa... so that was something to think about...

i ended up using 1password for 2fa, as I could put it on my computer and it didn't bother the login process terribly, but that is something you buy. Don't know if the other alternative I was given was any better; at that point I didn't much care as I was very confused by the whole process anyway and just wanted it over with lol...

I didn't care much if they had my phone number so I got the github app as a backup...

hope to see you around going forward 😄

@GitMensch
Copy link
Contributor

GitMensch commented Oct 5, 2023 via email

@GitMensch
Copy link
Contributor

I'm not sure why one would need (or even use) a proprietary 2 factor application when not explicit needed. Aegis works really well on Android https://getaegis.app/ and if running Windows 11 you can also use it via the Windows Subsystem for Android. On Linux Waydroid works quite fine, too.

So @chuggafan Are you still there?

@chuggafan
Copy link
Contributor Author

Apparently Github is doing a discord style "We will bother you once a week until you comply", which means I can continue to converse, which is nice.

In other news: I have begun on namespacing the entirety of ocpp in order to help with one of the issues I pointed out in my most recent commit's infodump.

@LADSoft
Copy link
Owner

LADSoft commented Aug 20, 2024

how are things looking?

@chuggafan
Copy link
Contributor Author

chuggafan commented Aug 20, 2024 via email

@LADSoft
Copy link
Owner

LADSoft commented Aug 22, 2024

yeah i get having to do other stuff, that is why things are going so dreadfully slowly for me too right now.

so the way namespaces are supposed to work is, when you use something like std::map or std::pair which uses __compressed pair, instantiating the outer template should use PushTemplateNameSpace() and PopTemplateNameSpace() to switch to the namespace the template was defined in so that references to other things in that namespace can be picked up. Maybe there is a hole in that logic somewhere... maybe if you start by selecting something via using statement it is broken? I'm going to leave it for you for right now but if you get tired of looking at it feel free to write an issue and I'll get to it lol...

@chuggafan
Copy link
Contributor Author

chuggafan commented Aug 24, 2024 via email

@LADSoft
Copy link
Owner

LADSoft commented Aug 24, 2024

the namespaces I mentioned above are associated with the function symbol... but while we are on the topic of arguments one of the things that is supposed to happen (in GetOverloadedFunction) is that the namespace associated with the argument is used as part of the lookup. so namespaces generally aren't a part of the callsite unless they are explicitly specified as part of the call... can't really put the namespace in the callsite without doing overload resolution as the same function signature may be found in multple namespaces....

One thing that may be happening (Im not sure it is as i don't remember the code well enough) is that if the argument type is a type defined in a class, it might not have an associated namespace as the namespace is associated with the class (or the outer class of a class tree). If I could verify the type of an argument was in the namespace I expect the function to be in that might be the issue... just a guess...

Templatereduce.cpp change TemplateDeduceArgList to actually check if the nested type has any data before getting any values out of it.

omake make it show where the command that failed is, should be made to be togglable and should ignore recursive make calls but I added it to track down where errors happen in massive parallel builds.

ppEmbed needed some changes simply to get it to a stage where it only really complains about <variant>

Compiler.h has added FALLTHROUGH because C++17 mandates warning about that, so deal with it.
@chuggafan
Copy link
Contributor Author

chuggafan commented Sep 23, 2024 via email

@LADSoft
Copy link
Owner

LADSoft commented Sep 23, 2024

yeah i fixed a lot of different stuff, it has been slow going because some of it is making me refactor established areas of the compiler... I've already had to refactor the using (non-aliased) directive twice in the last few weeks to cope with new aspects of C++17.... it was getting tiresome toward the end...

There are a couple of things left to do in terms of variants, the test program I grabbed from the web still won't compile. but I think mostly it is going to be a matter of bug fixes from here rather than dealing with new aspects of C++17... im going to be optimistic and hope I can get it done before this weekend :)

@chuggafan
Copy link
Contributor Author

chuggafan commented Sep 27, 2024 via email

@LADSoft
Copy link
Owner

LADSoft commented Sep 28, 2024

yeah probably nodes with ExpressionNode::templateselector_ shouldn't make it that far down the pipeline.

FWIW I have it in my plans that the next thing I'm going to do after I finally get my variant example to compile and run is review this branch and see if I can clean up any compiler bugs you are running into...

@LADSoft
Copy link
Owner

LADSoft commented Oct 3, 2024

Sorry im taking so long on the variant stuff, it is one thing after another.... i had to rework some more stuff and now I've got to do a small rewrite as to how 'auto' return types are done, because the way I did it didn't account for the fact you can chain auto-return function calls and it has to resolve to the innermost returned value sigh.... meanwhile i've needed to use my time for other things....

@chuggafan
Copy link
Contributor Author

chuggafan commented Oct 4, 2024 via email

@LADSoft
Copy link
Owner

LADSoft commented Oct 4, 2024

great that you are working on omake!

I wasn't sure from what you were saying whether you knew or whether it would even be relevant to what you are doing... but in CmdSwitch.cpp (in the src\util directory) there is code to convert a c++ string into an argv/argc sequence then parse it using the normal mechanisms.... it is used to parse config files and environment variables that have command line options from what I remember.

@chuggafan
Copy link
Contributor Author

chuggafan commented Oct 5, 2024 via email

@LADSoft
Copy link
Owner

LADSoft commented Oct 5, 2024

ok that sounds good. Whenever you get back to it, would you be open to putting that functionality into CmdSwitch.cpp so we have it in a common place?

@LADSoft
Copy link
Owner

LADSoft commented Oct 10, 2024

last weekend I fixed a bunch of bugs and did more rewrites; then I ran into a wall because the ellipse expansions are somewhat scatterbrained and need rework... I'm going to completely redesign that to make it work better.... see if i can get it done this weekend I guess....

@chuggafan
Copy link
Contributor Author

chuggafan commented Nov 24, 2024 via email

@LADSoft
Copy link
Owner

LADSoft commented Nov 26, 2024

so i fixed some stuff back in october but apparently never pushed it, I pushed it now but forgot to see if it builds first sigh... so im building...

BTW if you are in the US happy thanksgiving!

@chuggafan
Copy link
Contributor Author

chuggafan commented Nov 27, 2024 via email

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.

3 participants