JSON output improvements #813

Merged
merged 0 commits into from Oct 10, 2012

Conversation

Projects
None yet
9 participants
@ricochet1k

I have made many modifications to the JSON info generation built into DMD so that it produces nicely formatted json as well as a lot more information than it use to. There are still several things that I would like added to it, such as more detail on selective imports and import renaming, but I'm not sure how to get that information.

This should fix the following bugs:
http://d.puremagic.com/issues/show_bug.cgi?id=3404
http://d.puremagic.com/issues/show_bug.cgi?id=3466
http://d.puremagic.com/issues/show_bug.cgi?id=4178
http://d.puremagic.com/issues/show_bug.cgi?id=4194
http://d.puremagic.com/issues/show_bug.cgi?id=4477
http://d.puremagic.com/issues/show_bug.cgi?id=4478 (partial)

@alexrp

This comment has been minimized.

Show comment
Hide comment
@alexrp

alexrp Mar 14, 2012

Member

Please include the issue ID in the commit messages, e.g.: "Fix issue 3404 - <description, etc>"

Member

alexrp commented Mar 14, 2012

Please include the issue ID in the commit messages, e.g.: "Fix issue 3404 - <description, etc>"

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 14, 2012

Okay, done.

Okay, done.

@alexrp

This comment has been minimized.

Show comment
Hide comment
@alexrp

alexrp Mar 14, 2012

Member

Sorry for the late reply. I'm not sure if whatever regex it is we're using will pick up multiple issue references, so I suppose separate commits is the best approach...

Member

alexrp commented Mar 14, 2012

Sorry for the late reply. I'm not sure if whatever regex it is we're using will pick up multiple issue references, so I suppose separate commits is the best approach...

@braddr

This comment has been minimized.

Show comment
Hide comment
@braddr

braddr Mar 15, 2012

Member

The regex is: message_re = /((close|fix|address)e?(s|d)? )?(ticket|bug|tracker item|issue)s?:? *([\d ,+&#and]+)/i

That said, several small commits with specific changes is better than one big commit with a bunch of changes. Easier to review, easier to test in isolate, easier to revert if broken, etc.

Member

braddr commented Mar 15, 2012

The regex is: message_re = /((close|fix|address)e?(s|d)? )?(ticket|bug|tracker item|issue)s?:? *([\d ,+&#and]+)/i

That said, several small commits with specific changes is better than one big commit with a bunch of changes. Easier to review, easier to test in isolate, easier to revert if broken, etc.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 15, 2012

Ugh, why is the JSON output being used in a test? :(

Ugh, why is the JSON output being used in a test? :(

@braddr

This comment has been minimized.

Show comment
Hide comment
@braddr

braddr Mar 15, 2012

Member

Because it's a test OF the json output.

Member

braddr commented Mar 15, 2012

Because it's a test OF the json output.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 15, 2012

I know, I was mostly complaining that I didn't bother to run the tests myself.

I know, I was mostly complaining that I didn't bother to run the tests myself.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 15, 2012

Okay, so all the tests complete except for the one on Win32 because, for some reason, DMD adds some extra modifiers to the type of enum Y only on the Win32 platform. Json.c isn't doing anything different on different platforms, all it's doing is reporting information.

What should be done about it? Is this a separate DMD bug? I don't get why those storage class modifiers are platform specific.

Okay, so all the tests complete except for the one on Win32 because, for some reason, DMD adds some extra modifiers to the type of enum Y only on the Win32 platform. Json.c isn't doing anything different on different platforms, all it's doing is reporting information.

What should be done about it? Is this a separate DMD bug? I don't get why those storage class modifiers are platform specific.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Mar 15, 2012

Member

Can you be more specific what the problem is?

Member

WalterBright commented Mar 15, 2012

Can you be more specific what the problem is?

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 15, 2012

This is the diff the test generates of the expected json.out vs the one generated by the test on Win_32:

42c42,47
<                   "modifiers" : []
---
>                   "modifiers" : [
>                       "system",
>                       "ctfe",
>                       "disable",
>                       "nodefaultctor"
>                   ]

That modifiers array is simply a list of all the StorageClass flags that are set, generated by this function which is called from here. It may not be correct to be dumping all of them, but I figured more information is better than not enough.

On the Win_32 platform, those 4 storage class flags are set for the enum Y; when they aren't set on any of the other platforms.

This is the diff the test generates of the expected json.out vs the one generated by the test on Win_32:

42c42,47
<                   "modifiers" : []
---
>                   "modifiers" : [
>                       "system",
>                       "ctfe",
>                       "disable",
>                       "nodefaultctor"
>                   ]

That modifiers array is simply a list of all the StorageClass flags that are set, generated by this function which is called from here. It may not be correct to be dumping all of them, but I figured more information is better than not enough.

On the Win_32 platform, those 4 storage class flags are set for the enum Y; when they aren't set on any of the other platforms.

@yebblies

View changes

src/json.c
+ property("kind", "dchar");
+ // properties((TypeDchar *)type);
+ break;
+ case Terror:

This comment has been minimized.

@yebblies

yebblies Mar 16, 2012

Member

I don't think this should ever happen.

@yebblies

yebblies Mar 16, 2012

Member

I don't think this should ever happen.

This comment has been minimized.

@ricochet1k

ricochet1k Mar 16, 2012

It is one of the possible types though. Will it hurt to leave it in there?

@ricochet1k

ricochet1k Mar 16, 2012

It is one of the possible types though. Will it hurt to leave it in there?

This comment has been minimized.

@yebblies

yebblies Mar 16, 2012

Member

It means quietly semi-ignoring a bug in dmd (if Terror ever got here). Wouldn't it be better to fail loudly and quickly? I think it should really be replaced with an assert.

I also think all of the default: return "unknown"; cases should be replaced with asserts. This switch is missing a default too.

@yebblies

yebblies Mar 16, 2012

Member

It means quietly semi-ignoring a bug in dmd (if Terror ever got here). Wouldn't it be better to fail loudly and quickly? I think it should really be replaced with an assert.

I also think all of the default: return "unknown"; cases should be replaced with asserts. This switch is missing a default too.

This comment has been minimized.

@ricochet1k

ricochet1k Mar 16, 2012

Good point.

@yebblies

View changes

src/json.h
@@ -20,5 +20,63 @@
void json_generate(Modules *);
+struct JsonOut

This comment has been minimized.

@yebblies

yebblies Mar 16, 2012

Member

I don't think this is ever used outside of json.c, so it should probably be in there instead. These are effectively internal functions.

@yebblies

yebblies Mar 16, 2012

Member

I don't think this is ever used outside of json.c, so it should probably be in there instead. These are effectively internal functions.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Mar 19, 2012

Member

Is the Win32 issue resolved?

Member

WalterBright commented Mar 19, 2012

Is the Win32 issue resolved?

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Mar 19, 2012

Member

I don't want to pull this until the Win32 issue is resolved.

Member

WalterBright commented Mar 19, 2012

I don't want to pull this until the Win32 issue is resolved.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 19, 2012

No, the issue hasn't been resolved yet, but I'm positive it isn't the fault of this pull request. This pull only increases the information being shown, no more, and there is no platform specific code. There are StorageClass flags set in the Win32 build that aren't set on the other platforms, and I don't know why or where to look.

No, the issue hasn't been resolved yet, but I'm positive it isn't the fault of this pull request. This pull only increases the information being shown, no more, and there is no platform specific code. There are StorageClass flags set in the Win32 build that aren't set on the other platforms, and I don't know why or where to look.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Mar 19, 2012

Member

What is the minimum source code example that gets those attributes set on Windows?

Member

WalterBright commented Mar 19, 2012

What is the minimum source code example that gets those attributes set on Windows?

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 19, 2012

enum Y;

Taken from the test case json.d

enum Y;

Taken from the test case json.d

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Mar 20, 2012

Member

There are no storage classes for

enum Y;

EnumDeclaration doesn't even have a member with type StorageClass.

Member

WalterBright commented Mar 20, 2012

There are no storage classes for

enum Y;

EnumDeclaration doesn't even have a member with type StorageClass.

@WalterBright

View changes

src/json.c
+{
+ propertyStart(name);
+ arrayStart();
+

This comment has been minimized.

@WalterBright

WalterBright Mar 20, 2012

Member

See StorageClassDeclaration::stcToCBuffer(). The implementations should be shared. The storage classes not in stcToCBuffer() should not be printed, they are internal only.

@WalterBright

WalterBright Mar 20, 2012

Member

See StorageClassDeclaration::stcToCBuffer(). The implementations should be shared. The storage classes not in stcToCBuffer() should not be printed, they are internal only.

This comment has been minimized.

@ricochet1k

ricochet1k Mar 20, 2012

I did not know that existed, thanks! The only problem I have is that the ideal Json format is an array of the storage classes, which is incompatible with what stcToCBuffer prints. Should I put a comment in both places (or maybe only in json.c) pointing to where it was copied from? Alternatively, the SCstring table could be moved to an externally visible location so they could at least share that table.

@ricochet1k

ricochet1k Mar 20, 2012

I did not know that existed, thanks! The only problem I have is that the ideal Json format is an array of the storage classes, which is incompatible with what stcToCBuffer prints. Should I put a comment in both places (or maybe only in json.c) pointing to where it was copied from? Alternatively, the SCstring table could be moved to an externally visible location so they could at least share that table.

@WalterBright

View changes

src/json.c
+ propertyBool("property", type->isproperty);
+ propertyBool("ref", type->isref);
+
+ property("trust", TrustToChars(type->trust));

This comment has been minimized.

@WalterBright

WalterBright Mar 20, 2012

Member

Should not print this property if it is TRUSTdefault

@WalterBright

WalterBright Mar 20, 2012

Member

Should not print this property if it is TRUSTdefault

@WalterBright

View changes

src/json.c
+ propertyBool("ref", type->isref);
+
+ property("trust", TrustToChars(type->trust));
+ property("purity", PurityToChars(type->purity));

This comment has been minimized.

@WalterBright

WalterBright Mar 20, 2012

Member

Should not print purity if it is the default.

@WalterBright

WalterBright Mar 20, 2012

Member

Should not print purity if it is the default.

@WalterBright

View changes

src/json.c
+
+ property("trust", TrustToChars(type->trust));
+ property("purity", PurityToChars(type->purity));
+ property("linkage", LinkageToChars(type->linkage));

This comment has been minimized.

@WalterBright

WalterBright Mar 20, 2012

Member

Should not print linkage if it is D linkage.

@WalterBright

WalterBright Mar 20, 2012

Member

Should not print linkage if it is D linkage.

@WalterBright

View changes

src/json.c
+ propertyStart("modifiers");
+ arrayStart();
+
+ if (type->isConst()) item("const");

This comment has been minimized.

@WalterBright

WalterBright Mar 20, 2012

Member

This is reinventing MODtoBuffer().

@WalterBright

WalterBright Mar 20, 2012

Member

This is reinventing MODtoBuffer().

@WalterBright

View changes

src/json.c
+ // properties((TypeVoid *)type);
+ break;
+ case Tint8:
+ property("kind", "int8");

This comment has been minimized.

@WalterBright

WalterBright Mar 20, 2012

Member

int8 should be byte, etc. for the rest of the types. Use the D type names. In fact, use TypeBasic::dstring.

@WalterBright

WalterBright Mar 20, 2012

Member

int8 should be byte, etc. for the rest of the types. Use the D type names. In fact, use TypeBasic::dstring.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 20, 2012

Gah, I'm not used to C++'s unsafe casting. I assumed EnumDeclaration was a Declaration.

Gah, I'm not used to C++'s unsafe casting. I assumed EnumDeclaration was a Declaration.

@yebblies

This comment has been minimized.

Show comment
Hide comment
@yebblies

yebblies Mar 20, 2012

Member

This should probably be done with virtual functions, not switches and casting. Like the rest of dmd.

Member

yebblies commented Mar 20, 2012

This should probably be done with virtual functions, not switches and casting. Like the rest of dmd.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 20, 2012

I agree. I thought in the beginning it would be easier to get the pull request accepted if it didn't change most of the *.h files. That reasoning doesn't really make sense to me anymore, now that I have little a better idea of what I'm doing. I'll make that change.

I agree. I thought in the beginning it would be easier to get the pull request accepted if it didn't change most of the *.h files. That reasoning doesn't really make sense to me anymore, now that I have little a better idea of what I'm doing. I'll make that change.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 22, 2012

So the problem is that I want the storage classes to be stored as separate items in a Json array, not as a list of them separated by spaces, which is what stcToCBuffer does. The table array inside stcToCBuffer would need to be moved to a location that can be accessed from json.c. While trying to move it, I encountered a problem because the Id::* members are initialized by Id::initialize, which happens after the table is created. So I added a static StorageClassDeclaration::initialize member and called it after Id::initialize. Not terribly pretty, but I couldn't find a better way to do it.

So the problem is that I want the storage classes to be stored as separate items in a Json array, not as a list of them separated by spaces, which is what stcToCBuffer does. The table array inside stcToCBuffer would need to be moved to a location that can be accessed from json.c. While trying to move it, I encountered a problem because the Id::* members are initialized by Id::initialize, which happens after the table is created. So I added a static StorageClassDeclaration::initialize member and called it after Id::initialize. Not terribly pretty, but I couldn't find a better way to do it.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 22, 2012

Don't pull yet! The inner function in the test case doesn't get dumped yet, I'm still investigating how to do that.

Don't pull yet! The inner function in the test case doesn't get dumped yet, I'm still investigating how to do that.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 22, 2012

I'm having a hard time finding a good way to recurse through all the inner declarations of a FuncDeclaration. The easiest way I can see is adding another visitor method to all Statement and Expression classes to get to the DeclarationExp class. It's going to have to wait unless someone has an idea.

I'm having a hard time finding a good way to recurse through all the inner declarations of a FuncDeclaration. The easiest way I can see is adding another visitor method to all Statement and Expression classes to get to the DeclarationExp class. It's going to have to wait unless someone has an idea.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 23, 2012

In case I wasn't clear, this pull is ready, even though it could have more functionality.

In case I wasn't clear, this pull is ready, even though it could have more functionality.

@braddr

This comment has been minimized.

Show comment
Hide comment
@braddr

braddr Mar 23, 2012

Member

This change would likely be considerably less code if it used the apply infrastructure. See also dmd pull #825 that I just submitted to add apply support to ArrayBase.

Member

braddr commented Mar 23, 2012

This change would likely be considerably less code if it used the apply infrastructure. See also dmd pull #825 that I just submitted to add apply support to ArrayBase.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Mar 23, 2012

There are a few places that using apply would make this a little smaller, but not that many. What would be really nice is a Visitor pattern for all Statements, Expressions, Declarations, etc.

There are a few places that using apply would make this a little smaller, but not that many. What would be really nice is a Visitor pattern for all Statements, Expressions, Declarations, etc.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 29, 2012

Member

Thanks for your effort, but let's try to break this down into smaller pieces.
At the current size it's impossible for anyone to assess.

Member

MartinNowak commented Apr 29, 2012

Thanks for your effort, but let's try to break this down into smaller pieces.
At the current size it's impossible for anyone to assess.

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k May 1, 2012

I did make a lot of changes to this, that's for sure. How about I break it into one commit that is the massive upgrade I did to the formatting, and then commits after for each bug fixed? Would that be better?

I did make a lot of changes to this, that's for sure. How about I break it into one commit that is the massive upgrade I did to the formatting, and then commits after for each bug fixed? Would that be better?

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak May 2, 2012

Member

Sounds reasonable.

Member

MartinNowak commented May 2, 2012

Sounds reasonable.

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex

andralex Sep 25, 2012

Member

@ricochet1k what's the status of this bad boy?

Member

andralex commented Sep 25, 2012

@ricochet1k what's the status of this bad boy?

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Sep 25, 2012

Hmm, I thought I cleaned this up. Rewriting history is a bit of a pain. I'll try to take another look soon.

Hmm, I thought I cleaned this up. Rewriting history is a bit of a pain. I'll try to take another look soon.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 4, 2012

Member

Please don't let this die "just" because of how the commits are structured. The changes are desperately needed and from what the final diff looks like, there are a lot of uniform changes and it should be reviewable by taking one or two hours off (vs. possibly many hours needed to take apart and restructure everything).

Some practical solution to this would be good. My feeling is that the harm is possibly greater for not having it, even than having it with maybe some bugs.

Member

s-ludwig commented Oct 4, 2012

Please don't let this die "just" because of how the commits are structured. The changes are desperately needed and from what the final diff looks like, there are a lot of uniform changes and it should be reviewable by taking one or two hours off (vs. possibly many hours needed to take apart and restructure everything).

Some practical solution to this would be good. My feeling is that the harm is possibly greater for not having it, even than having it with maybe some bugs.

src/attrib.c
@@ -499,7 +502,7 @@ void StorageClassDeclaration::stcToCBuffer(OutBuffer *buf, StorageClass stc)
{ STCalias, TOKalias },
{ STCout, TOKout },
{ STCin, TOKin },
-#if DMDV2
+ #if DMDV2

This comment has been minimized.

@nazriel

nazriel Oct 7, 2012

Contributor

There should be no tab if we stick to DMD src style, or there was a particular reason to add it?

@nazriel

nazriel Oct 7, 2012

Contributor

There should be no tab if we stick to DMD src style, or there was a particular reason to add it?

src/attrib.c
@@ -513,19 +516,25 @@ void StorageClassDeclaration::stcToCBuffer(OutBuffer *buf, StorageClass stc)
{ STCtrusted, TOKat, Id::trusted },
{ STCsystem, TOKat, Id::system },
{ STCdisable, TOKat, Id::disable },
-#endif
+ #endif

This comment has been minimized.

@nazriel

nazriel Oct 7, 2012

Contributor

ditto

@nazriel

nazriel Oct 7, 2012

Contributor

ditto

@nazriel

This comment has been minimized.

Show comment
Hide comment
@nazriel

nazriel Oct 7, 2012

Contributor

@ricochet1k this is really cool. For sure will be useful. Could you check in what is going on with Pull-tester failures and rebase this bastard?

Thanks!

Contributor

nazriel commented Oct 7, 2012

@ricochet1k this is really cool. For sure will be useful. Could you check in what is going on with Pull-tester failures and rebase this bastard?

Thanks!

@braddr

This comment has been minimized.

Show comment
Hide comment
@braddr

braddr Oct 7, 2012

Member

I think this request needs to be broken up as well. Small obviously correct changes tend to be pulled far faster than huge sprawling changes. I think everyone is in general agreement that this set of changes is a good one, just need to find a way to give it a good review

Member

braddr commented Oct 7, 2012

I think this request needs to be broken up as well. Small obviously correct changes tend to be pulled far faster than huge sprawling changes. I think everyone is in general agreement that this set of changes is a good one, just need to find a way to give it a good review

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Oct 8, 2012

@nazriel I'm not sure why those were indented.

I'll try to rebase and fix it tonight, and probably merge all non-bugfix commits into one.

@nazriel I'm not sure why those were indented.

I'll try to rebase and fix it tonight, and probably merge all non-bugfix commits into one.

@ricochet1k ricochet1k merged commit 649ad79 into dlang:master Oct 10, 2012

@ricochet1k

This comment has been minimized.

Show comment
Hide comment
@ricochet1k

ricochet1k Oct 10, 2012

Oops, didn't mean to close this. Github closed it when I pushed. I'll create a new one when I get it finished.

Oops, didn't mean to close this. Github closed it when I pushed. I'll create a new one when I get it finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment