You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2022/10/14 20:48:32 UTC
[GitHub] [thrift] Jens-G opened a new pull request, #2714: THRIFT-5652 IDL uuid literals can be improved
Jens-G opened a new pull request, #2714:
URL: https://github.com/apache/thrift/pull/2714
Patch: Jens Geyer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G closed pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
Jens-G closed pull request #2714: THRIFT-5652 IDL uuid literals can be improved
URL: https://github.com/apache/thrift/pull/2714
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
fishy commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996171062
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
several reasons:
1. This would likely become a very common mistake, that developers would think that they are supposed to write it in this way: `const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"`
2. The editor supports for the new syntax will take time to catch up (see how github is highlighting the syntax right now for an example), which will further make 1 more likely to happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
Jens-G commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996163281
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
> an we force it still to be in string format? (e.g. adding leading and trailing " to the ll file)
Why? From a syntactical view I see no necessity for this.
Probably yes, but then I would use *only* the string form. I had to fight it already to get curly/noncurly working.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
fishy commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996141857
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
1. can we force it still to be in string format? (e.g. adding leading and trailing `"` to the ll file)
2. ~~if we decided that we want to support the form with curly braces, then I'll need to add support to that to the go library.~~
3. ~~will the compiler still take this as a string token?~~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
fishy commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996150478
##########
compiler/cpp/src/thrift/thriftl.ll:
##########
@@ -248,6 +250,18 @@ literal_begin (['\"])
}
"&" { return tok_reference; }
+{uuidconstant} {
+ /* Placed before intconstant, because uuids start with either "{" or directly with some hex digit */
+ yylval.uconst = strdup(yytext);
+ return tok_uuid_constant;
+}
+
+{guidconstant} {
+ yylval.uconst = strdup(&yytext[1]);
+ yylval.uconst[36] = '\000';
+ return tok_uuid_constant;
+}
Review Comment:
oh I see you stripped the curly braces from the token so language libraries do not need to support the parsing of this form.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
Jens-G commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996279002
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
Done. That would have saved me some time if we had made this clear from the beginning.
PS. I'm going to squash before merge, I just want to keep it this way for clarity during review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
Jens-G commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996279002
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
Done. That would have saved me some time if we had made this clear from the beginning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
Jens-G commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996163281
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
> an we force it still to be in string format? (e.g. adding leading and trailing " to the ll file)
Why? Probably yes, but then I would use *only* the string form. I had to fight it already to get curly/noncurly working.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
Jens-G commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996273524
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
We can't fly to Mars because we don't have syntax highlighting for it .... :-D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
fishy commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996345892
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
Mars is overrated anyways :-D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
Jens-G commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996163281
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
> an we force it still to be in string format? (e.g. adding leading and trailing " to the ll file)
Why?
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
> an we force it still to be in string format? (e.g. adding leading and trailing " to the ll file)
Why?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a diff in pull request #2714: THRIFT-5652 IDL uuid literals can be improved
Posted by GitBox <gi...@apache.org>.
fishy commented on code in PR #2714:
URL: https://github.com/apache/thrift/pull/2714#discussion_r996141857
##########
test/ConstantsDemo.thrift:
##########
@@ -65,7 +65,9 @@ const map<i32,thing> GEN_WHAT = { 35 : { 'hello' : 325, 'goodbye' : 325352 } }
const set<i32> GEN_SET = [ 235, 235, 53235 ]
-const uuid GEN_UUID = "00000000-4444-CCCC-ffff-0123456789ab"
+// uuids are accepted with or without curly braces
+const uuid GEN_UUID = 00000000-4444-CCCC-ffff-0123456789ab
+const uuid GEN_GUID = {00112233-4455-6677-8899-aaBBccDDeeFF}
Review Comment:
1. can we force it still to be in string format? (e.g. adding leading and trailing `"` to the ll file)
2. if we decided that we want to support the form with curly braces, then I'll need to add support to that to the go library.
3. will the compiler still take this as a string token?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org