Details

    • Patch Info:
      Patch Available

      Description

      This introduces support for the final keyword in the thrift IDL. A C++ thrift struct that is declared final loses it's virtual dtor, which saves the memory overhead of one vtable per instance. This (along with the required keyword) can be very helpful if you're passing around big lists of small thrift structs.

      This patch also includes modifications for the java generator to recognize and apply final. I'm not familiar enough with the other languages to know if this notion applies to them.

      (A patch for this was submitted in thrift's pre-apache days and was met with approval, but then fell off the radar, so I'm trying again.)

      1. final_annotation_cpp_java_csharp_v2.patch
        3 kB
        Erik Frey
      2. final_annotation_cpp_java_csharp_v3.patch
        3 kB
        Erik Frey
      3. final_annotation_cpp_java_csharp.patch
        2 kB
        Erik Frey
      4. final_csharp.patch
        0.5 kB
        Michael Greene
      5. keyword_final_cpp_java.patch
        4 kB
        Erik Frey

        Activity

        Hide
        Erik Frey added a comment -

        Keyword final patch for lexer, parser, c++ generator, java generator.

        Show
        Erik Frey added a comment - Keyword final patch for lexer, parser, c++ generator, java generator.
        Hide
        Bryan Duxbury added a comment -

        This seems cool. I almost think this should be the default for the java generation, since I would imagine very few users derive more classes from the generated Thrift structs.

        I think the patch looks pretty good, at least the java implementation stuff. I don't have much input about the lexer/parser/c++ stuff.

        Show
        Bryan Duxbury added a comment - This seems cool. I almost think this should be the default for the java generation, since I would imagine very few users derive more classes from the generated Thrift structs. I think the patch looks pretty good, at least the java implementation stuff. I don't have much input about the lexer/parser/c++ stuff.
        Hide
        Michael Greene added a comment -

        C# has an analog in the sealed keyword.
        Attaching a patch for the C# generator to apply sealed when a thrift struct is declared final.

        Show
        Michael Greene added a comment - C# has an analog in the sealed keyword. Attaching a patch for the C# generator to apply sealed when a thrift struct is declared final.
        Hide
        David Reiss added a comment -

        Seems like a useful feature. I'd rather see it implemented in terms of type annotations than as a keyword, though. I should have that feature ready this afternoon.

        Show
        David Reiss added a comment - Seems like a useful feature. I'd rather see it implemented in terms of type annotations than as a keyword, though. I should have that feature ready this afternoon.
        Hide
        Bryan Duxbury added a comment -

        What's the status of this? David, did you ever get the type annotations stuff done?

        Show
        Bryan Duxbury added a comment - What's the status of this? David, did you ever get the type annotations stuff done?
        Hide
        David Reiss added a comment -

        Yep. It's hanging out in https://issues.apache.org/jira/browse/THRIFT-121, waiting for a review. I never got around to making a less hacky test, but it appears to work.

        Show
        David Reiss added a comment - Yep. It's hanging out in https://issues.apache.org/jira/browse/THRIFT-121 , waiting for a review. I never got around to making a less hacky test, but it appears to work.
        Hide
        Jérémie BORDIER added a comment -

        Is this ticket still open ? THRIFT-121 is now closed and commited.

        Show
        Jérémie BORDIER added a comment - Is this ticket still open ? THRIFT-121 is now closed and commited.
        Hide
        Erik Frey added a comment -

        Okay, here's a patch! It adds cpp.final, java.final, and csharp.final annotations to their respective generators.

        Since annotations are generator-specific, I considered something like cpp.nonvirtual, java.final, and csharp.sealed, but this felt a bit like overkill. Feel free to alter the patch if you feel that's more appropriate.

        Show
        Erik Frey added a comment - Okay, here's a patch! It adds cpp.final, java.final, and csharp.final annotations to their respective generators. Since annotations are generator-specific, I considered something like cpp.nonvirtual, java.final, and csharp.sealed, but this felt a bit like overkill. Feel free to alter the patch if you feel that's more appropriate.
        Hide
        David Reiss added a comment -

        For the Java and C# ones, I'd prefer to see '''(tstruct->annotations_.find("csharp.final") != tstruct->annotations_.end() ? "sealed " : "")''' captured as a separate variable first, rather than in the middle of the print.

        Also, it is not necessary for the annotations to be prefixed with languages. I thought it would be a nice convention, but if certain annotations are universally (or nearly universally) applicable, we can drop it for those.

        Personally, I think I prefer, cpp.nonvirtual, java.final, and csharp.sealed, but I don't feel very strongly about it.

        Thoughts?

        Show
        David Reiss added a comment - For the Java and C# ones, I'd prefer to see '''(tstruct->annotations_.find("csharp.final") != tstruct->annotations_.end() ? "sealed " : "")''' captured as a separate variable first, rather than in the middle of the print. Also, it is not necessary for the annotations to be prefixed with languages. I thought it would be a nice convention, but if certain annotations are universally (or nearly universally) applicable, we can drop it for those. Personally, I think I prefer, cpp.nonvirtual, java.final, and csharp.sealed, but I don't feel very strongly about it. Thoughts?
        Hide
        Michael Greene added a comment -

        I would prefer either "final" and have that apply to cpp/java/csharp if the behavior is substantially the same between the languages, or language-specific names if they are going to be prefixed.

        i.e. either
        final (for all languages)
        or
        cpp.nonvirtual, java.final, and csharp.sealed

        I lean towards just "final" but don't have a strong preference either.

        Show
        Michael Greene added a comment - I would prefer either "final" and have that apply to cpp/java/csharp if the behavior is substantially the same between the languages, or language-specific names if they are going to be prefixed. i.e. either final (for all languages) or cpp.nonvirtual, java.final, and csharp.sealed I lean towards just "final" but don't have a strong preference either.
        Hide
        Bryan Duxbury added a comment -

        I think just "final" is way simpler.

        Show
        Bryan Duxbury added a comment - I think just "final" is way simpler.
        Hide
        Erik Frey added a comment -

        Okay, here's a final final annotation patch

        Includes suggestion to move the map.find to a separate line for clarity, and to use "final" for consistency across languages (not to mention forward compatibility with any other generators that may eventually support it).

        Note that to use "final" I had to remove it from the lexer definition, where it had been declared as a reserved word.

        Show
        Erik Frey added a comment - Okay, here's a final final annotation patch Includes suggestion to move the map.find to a separate line for clarity, and to use "final" for consistency across languages (not to mention forward compatibility with any other generators that may eventually support it). Note that to use "final" I had to remove it from the lexer definition, where it had been declared as a reserved word.
        Hide
        Bryan Duxbury added a comment -

        Hey, I'd love to get this patch applied. I'll commit to reviewing the Java portion. Can others chime in with reviews for their respective libraries?

        Show
        Bryan Duxbury added a comment - Hey, I'd love to get this patch applied. I'll commit to reviewing the Java portion. Can others chime in with reviews for their respective libraries?
        Hide
        Michael Greene added a comment -

        Yes. I have not yet reviewed the latest patch, I will chime in later.

        Show
        Michael Greene added a comment - Yes. I have not yet reviewed the latest patch, I will chime in later.
        Hide
        Jérémie BORDIER added a comment -

        Looks allright to me. +1

        Show
        Jérémie BORDIER added a comment - Looks allright to me. +1
        Hide
        David Reiss added a comment -

        Bummer about having to remove the keyword, but we should probably be doing that filtering at a higher level than the parser anyway.

        Show
        David Reiss added a comment - Bummer about having to remove the keyword, but we should probably be doing that filtering at a higher level than the parser anyway.
        Hide
        David Reiss added a comment -

        The csharp portion doesn't apply. Can you svn up, resolve the conflicts, re-test, and re-upload the patch?

        Show
        David Reiss added a comment - The csharp portion doesn't apply. Can you svn up, resolve the conflicts, re-test, and re-upload the patch?
        Hide
        Bryan Duxbury added a comment -

        The java portion looks good. (Didn't realize this was such a short patch.)

        Show
        Bryan Duxbury added a comment - The java portion looks good. (Didn't realize this was such a short patch.)
        Hide
        Erik Frey added a comment -

        Patch correctly applies to csharp generator.

        Show
        Erik Frey added a comment - Patch correctly applies to csharp generator.
        Hide
        Bryan Duxbury added a comment -

        Anything keeping this from being committed? I tested it and it applies cleanly now.

        Show
        Bryan Duxbury added a comment - Anything keeping this from being committed? I tested it and it applies cleanly now.
        Hide
        David Reiss added a comment -

        Bummer about having to change the lexer. Though I guess in the long term, per-language keywords should be blocked at a higher level. Ship it.

        Show
        David Reiss added a comment - Bummer about having to change the lexer. Though I guess in the long term, per-language keywords should be blocked at a higher level. Ship it.
        Hide
        Michael Greene added a comment -

        Current csharp patch looks good to me. Haven't had the time to put it through its paces, but there's not a lot to it so +1.

        Show
        Michael Greene added a comment - Current csharp patch looks good to me. Haven't had the time to put it through its paces, but there's not a lot to it so +1.
        Hide
        Bryan Duxbury added a comment -

        Committed. Thanks for the patch Erik!

        Show
        Bryan Duxbury added a comment - Committed. Thanks for the patch Erik!

          People

          • Assignee:
            Erik Frey
            Reporter:
            Erik Frey
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development