Enhance AST dump about gccrs HOT 12 CLOSED

I really like this output it makes it more in like with how -fdump-tree-gimple looks also

from gccrs.

There's two main comments I'd like to make here:

• Some of the non-dump-related code uses the as_string() method to convert some things into string form, such as paths (e.g. converting something::something_else to "something::something_else"). As such, if you convert all object dumping processes to output the same form, either the code requesting a string form would have to be changed (i.e. new method), or you might have to add a new method like to_dump_string() or something.

• The main issue that I experienced with creating a nice-looking dump is that indentation levels are pretty much impossible to create with the current system of recursive as_string() calls, as the called object has no conception of the indentation level of the parent, which prevents any indentation from being preserved if the called object adds a new line to the string. You'd probably have to pretty heavily modify the dumping mechanism to do this - the best solution that I can currently think of would be a separate "Dump Creator" object that holds state on the current indentation level and the string so far, and a theoretical dump_string() method could add new lines to that object, and then the final string could be extracted from it at the end. This would probably be a lot of work and may involve unforeseen issues, however.

There's also one minor nitpick with the current example you have shown - strictly speaking, BlockExpr includes the curly braces surrounding it (as that is how BlockExpr is defined).

from gccrs.

@SimplyTheOther
I don't expect to make it perfect in a short time. Actually, I think we need a more readable output to help us to debug the very beginning syntax parsing, and this may help to attract more contributions. So we may improve it gradually. Of course, we could also rewrite the AST (iff it's not good enough) in the future for better dumping work. If the indentation is finally proved to be a problem, we may have to use an external indent tool to help us. After all, it's just for the very beginning debugging.

Thanks for the mention. I haven't read BlockExpr part. I only tried to improve the function and parameters for a prove of concept. I'll keep updating it.

from gccrs.

BTW, if you're going to create a PR for indentation, please consider merging this branch first to avoid too many conflicts, then indent the whole code. I'm sorry it's still half baked, but I don't think it will break existing things at least.

However, here's a thing I have to mention. I've patched the string literal token to add double-quote to the value. I think this should be done when lexing the string literal. Otherwise, there are no double quotes around string when we print them, say println!(Hello World!)

from gccrs.

I've updated for indentation, so far so good. However, the global variable has to be used since the current AST doesn't support the indentation well. But I think it's fine. We don't have to spend time optimizing AST dump. It's just for debugging support. Let's save time for type checking and IR transformation.

Crate: 
 inner attributes: none
 items: 
  
u32 abc(x : u32, y : u32)
BlockExpr:
{
 outer attributes: none
 inner attributes: none
 statements: 
 ExprStmtWithoutBlock: return ArithmeticOrLogicalExpr: x + y
 final expression: none
}

  
void main()
BlockExpr:
{
 outer attributes: none
 inner attributes: none
 statements: 
  ExprStmtWithBlock: 
   BlockExpr:
   {
    outer attributes: none
    inner attributes: none
    statements: 
    ExprStmtWithoutBlock: ArithmeticOrLogicalExpr: 1 + 1
    final expression: none
   }
 ExprStmtWithoutBlock: println!("Hello World!")
 final expression: none
}

from gccrs.

@NalaGinrut @philberty Kind of off-topic, but do you think this is a good template to show others what we’re working on at the moment (i.e. create an issue)? Or is there a better way (e.g. draft pull requests, projects maybe)?

BTW, if you're going to create a PR for indentation, please consider merging this branch first to avoid too many conflicts, then indent the whole code. I'm sorry it's still half baked, but I don't think it will break existing things at least.

However, here's a thing I have to mention. I've patched the string literal token to add double-quote to the value. I think this should be done when lexing the string literal. Otherwise, there are no double quotes around string when we print them, say println!(Hello World!)

Depending on what exactly you mean by that (i.e. when you add the double quotes - at the lexer stage or later), that might break some of the code comparing the values of string literals (e.g. cfg attribute handling). I think it would be better to store the value of the string value only and then add the double quotes in the as_string() method, if that’s not what you’ve done already (since adding characters is easier than removing them).

from gccrs.

@philberty @SimplyTheOther

1. If you want to show AST dump to promote gccrs, I think it's at least an explicit and understandable work to common users. It shows the parser work for Rust syntax. Personally, I think it's OK.
   The only concern is the time. I don't think it can be complete for every corner case of Rust syntax in a short time. However, it's not good to keep it unmerged for a long time since it should be helpful for our next step development. How about this, I will raise a PR after a few tweaks. Then we can promote gccrs by indicating AST dump, and we can fix the indentation issue after this PR soon.

2. Adding necessary double-quotes would be better, however, I didn't find the correct place in AST dump. I've tried some, but it doesn't work. I think it's fine. I can remove the lexer modification, and postpone this tiny fix.

from gccrs.

@philberty @SimplyTheOther

1. If you want to show AST dump to promote gccrs, I think it's at least an explicit and understandable work to common users. It shows the parser work for Rust syntax. Personally, I think it's OK.
   The only concern is the time. I don't think it can be complete for every corner case of Rust syntax in a short time. However, it's not good to keep it unmerged for a long time since it should be helpful for our next step development. How about this, I will raise a PR after a few tweaks. Then we can promote gccrs by indicating AST dump, and we can fix the indentation issue after this PR soon.

I see your reasoning. I'm just a little wary about the possibility of constructing a system that is difficult to change afterwards and having to do massive refactoring at some point in the future. But yeah, right now a predominantly working parser is probably more important.

2. Adding necessary double-quotes would be better, however, I didn't find the correct place in AST dump. I've tried some, but it doesn't work. I think it's fine. I can remove the lexer modification, and postpone this tiny fix.

You would add the double quotes in the as_string() method in Literal in rust-ast.h if the literal type is a string (so you'd probably do a switch, since characters should also have single quotes too then). I would then also add a raw_value() method (or some other name) that returns the raw value_as_string so that comparisons with raw value don't have to strip the quotes off.

from gccrs.

@philberty @SimplyTheOther

1. Yes, although we can rethink many things at this stage, there're a lot of other works to do. I think we can polish in the future. If you were asking about the WIP template, I think the answer is yes. For larger work, it's better to raise an issue with WIP, and contributors may follow the related branch. I never used project, I don't know if it's proper for the case.

2. For the string literal part, I have removed my modification in the PR. I may need some time to figure it out. We don't have to fix it in the same PR.

from gccrs.

Oh, I just tried projects, there's kanban, and we can drag related issues in. I think we can take advantage of it.

from gccrs.

I think PR's should be for things you want to get merged into Master when you think they are ready, otherwise in my experience PR's have a tendency to be neglected by reviewers.

For distributing work i started to create a github kanban board i am not sure if you guys have access to create/update on there if not i will take a look at fix that for you guys. I think we should use itas much as we can if a piece of work is of bigger scope or are not sure about putting on the kanban board lets create an issue to discuss it.

What do you think? I am happy to change things but lets try this for now for a few weeks.

from gccrs.

@philberty I think the Github kanban can be based on issues, say, you can just reference the issue number with a hash to reference, you don't even have to write any description. Then we can connect kanban and issues together.

However, if the new feature is working by just one person, the kanban could be ignored, the issue is enough. But if we want to attract more people to work together on the same sub-project, the kanban is still important.

So I think, at least in this stage, maybe kanban is not necessary, depends on you whether to use it. Personally, I'm glad to try. But issues should play an important role in any time.

from gccrs.