Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

JSON output improvements #813

Merged
merged 0 commits into from over 1 year ago

9 participants

Matt Peterson Alex Rønne Petersen Brad Roberts Walter Bright Daniel Murphy Martin Nowak Andrei Alexandrescu Sönke Ludwig Damian Ziemba
Matt Peterson

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)

Alex Rønne Petersen

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

Matt Peterson

Okay, done.

Alex Rønne Petersen

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...

Brad Roberts
Owner

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.

Matt Peterson

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

Brad Roberts
Owner

Because it's a test OF the json output.

Matt Peterson

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

Matt Peterson

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.

Walter Bright
Owner

Can you be more specific what the problem is?

Matt Peterson

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.

src/json.c
((580 lines not shown))
  707
+            property("kind", "bool");
  708
+            // properties((TypeBool *)type);
  709
+            break;
  710
+        case Tchar:
  711
+            property("kind", "char");
  712
+            // properties((TypeChar *)type);
  713
+            break;
  714
+        case Twchar:
  715
+            property("kind", "wchar");
  716
+            // properties((TypeWchar *)type);
  717
+            break;
  718
+        case Tdchar:
  719
+            property("kind", "dchar");
  720
+            // properties((TypeDchar *)type);
  721
+            break;
  722
+        case Terror:
4
Daniel Murphy Collaborator
yebblies added a note March 15, 2012

I don't think this should ever happen.

Matt Peterson
ricochet1k added a note March 15, 2012

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

Daniel Murphy Collaborator
yebblies added a note March 15, 2012

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.

Matt Peterson
ricochet1k added a note March 15, 2012

Good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/json.h
... ...
@@ -20,5 +20,63 @@
20 20
 
21 21
 void json_generate(Modules *);
22 22
 
  23
+struct JsonOut
1
Daniel Murphy Collaborator
yebblies added a note March 15, 2012

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.

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

Is the Win32 issue resolved?

Walter Bright
Owner

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

Matt Peterson

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.

Walter Bright
Owner

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

Matt Peterson
enum Y;

Taken from the test case json.d

Walter Bright
Owner

There are no storage classes for

enum Y;

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

src/json.c
((190 lines not shown))
  377
+            return "cpp";
  378
+        case LINKwindows:
  379
+            return "windows";
  380
+        case LINKpascal:
  381
+            return "pascal";
  382
+        default:
  383
+            assert(false);
  384
+    }
  385
+}
  386
+
  387
+
  388
+void JsonOut::propertyStorageClass(const char *name, StorageClass stc)
  389
+{
  390
+    propertyStart(name);
  391
+    arrayStart();
  392
+
2
Walter Bright Owner

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

Matt Peterson
ricochet1k added a note March 19, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/json.c
((323 lines not shown))
  510
+{
  511
+    property("next", type->next);
  512
+}
  513
+
  514
+void JsonOut::properties(TypeReference *type)
  515
+{
  516
+    property("next", type->next);
  517
+}
  518
+
  519
+void JsonOut::properties(TypeFunction *type)
  520
+{
  521
+    propertyBool("nothrow", type->isnothrow);
  522
+    propertyBool("property", type->isproperty);
  523
+    propertyBool("ref", type->isref);
  524
+
  525
+    property("trust", TrustToChars(type->trust));
1
Walter Bright Owner

Should not print this property if it is TRUSTdefault

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/json.c
((324 lines not shown))
  511
+    property("next", type->next);
  512
+}
  513
+
  514
+void JsonOut::properties(TypeReference *type)
  515
+{
  516
+    property("next", type->next);
  517
+}
  518
+
  519
+void JsonOut::properties(TypeFunction *type)
  520
+{
  521
+    propertyBool("nothrow", type->isnothrow);
  522
+    propertyBool("property", type->isproperty);
  523
+    propertyBool("ref", type->isref);
  524
+
  525
+    property("trust", TrustToChars(type->trust));
  526
+    property("purity", PurityToChars(type->purity));
1
Walter Bright Owner

Should not print purity if it is the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/json.c
((325 lines not shown))
  512
+}
  513
+
  514
+void JsonOut::properties(TypeReference *type)
  515
+{
  516
+    property("next", type->next);
  517
+}
  518
+
  519
+void JsonOut::properties(TypeFunction *type)
  520
+{
  521
+    propertyBool("nothrow", type->isnothrow);
  522
+    propertyBool("property", type->isproperty);
  523
+    propertyBool("ref", type->isref);
  524
+
  525
+    property("trust", TrustToChars(type->trust));
  526
+    property("purity", PurityToChars(type->purity));
  527
+    property("linkage", LinkageToChars(type->linkage));
1
Walter Bright Owner

Should not print linkage if it is D linkage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/json.c
((429 lines not shown))
  616
+
  617
+
  618
+void JsonOut::property(const char *name, Type *type)
  619
+{
  620
+    if (type == NULL) return;
  621
+
  622
+    propertyStart(name);
  623
+    objectStart();
  624
+
  625
+    property("raw", type->toChars());
  626
+
  627
+
  628
+    propertyStart("modifiers");
  629
+    arrayStart();
  630
+
  631
+    if (type->isConst()) item("const");
1
Walter Bright Owner

This is reinventing MODtoBuffer().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/json.c
((497 lines not shown))
  684
+            properties((TypeTypedef *)type);
  685
+            break;
  686
+        case Tdelegate:
  687
+            property("kind", "delegate");
  688
+            properties((TypeDelegate *)type);
  689
+            break;
  690
+        case Tnone:
  691
+            property("kind", "none");
  692
+            // properties((TypeNone *)type);
  693
+            break;
  694
+        case Tvoid:
  695
+            property("kind", "void");
  696
+            // properties((TypeVoid *)type);
  697
+            break;
  698
+        case Tint8:
  699
+            property("kind", "int8");
1
Walter Bright Owner

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

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

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

Daniel Murphy
Collaborator

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

Matt Peterson

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.

Matt Peterson

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.

Matt Peterson

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

Matt Peterson

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.

Matt Peterson

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

Brad Roberts
Owner

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.

Matt Peterson

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.

Martin Nowak
Collaborator

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.

Matt Peterson

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?

Martin Nowak
Collaborator

Sounds reasonable.

Andrei Alexandrescu
Owner

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

Matt Peterson

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

Sönke Ludwig

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)
499 502
         { STCalias,        TOKalias },
500 503
         { STCout,          TOKout },
501 504
         { STCin,           TOKin },
502  
-#if DMDV2
  505
+    #if DMDV2
1
Damian Ziemba
nazriel added a note October 07, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/attrib.c
... ...
@@ -513,19 +516,25 @@ void StorageClassDeclaration::stcToCBuffer(OutBuffer *buf, StorageClass stc)
513 516
         { STCtrusted,      TOKat,       Id::trusted },
514 517
         { STCsystem,       TOKat,       Id::system },
515 518
         { STCdisable,      TOKat,       Id::disable },
516  
-#endif
  519
+    #endif
1
Damian Ziemba
nazriel added a note October 07, 2012

ditto

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

@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!

Brad Roberts
Owner

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

Matt Peterson

@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.

Matt Peterson ricochet1k merged commit 649ad79 into from October 10, 2012
Matt Peterson ricochet1k closed this October 10, 2012
Matt Peterson

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

Brad Roberts braddr referenced this pull request from a commit October 21, 2012
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Sorry, commit information is not available for this pull request.

This page is out of date. Refresh to see the latest.

Showing 0 changed files with 0 additions and 0 deletions. Show diff stats Hide diff stats

Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.