Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes related to the use with MS Visual Studio Compiler #171

Closed
wants to merge 2 commits into from
Closed

Fixes related to the use with MS Visual Studio Compiler #171

wants to merge 2 commits into from

Conversation

slavanap
Copy link

Additional overflow check (zero_copy_stream_impl.cc)

Additional security check (zero_copy_stream_impl.cc),
Fix in protoc for MSVC: VC requires explicit declaration of constructor for struct $name$OneofInstance() (cpp_message.cc)
@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@slavanap
Copy link
Author

I have signed CLA.

@googlebot
Copy link

CLAs look good, thanks!

@@ -1217,6 +1217,11 @@ GenerateDescriptorDeclarations(io::Printer* printer) {
}
}

printer->Print("#ifdef _MSC_VER" "\n"
" $name$OneofInstance() {}" "\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would VC require a constructor? I tested v3.0.0-alpha-1 on visual studio 2008 and 2013 but did not find such a problem.

@slavanap
Copy link
Author

My bad that I used commit
11/21/2014 03:18:53 SHA-1: 99aa0f9 "Down-integrate from internal code base." as a reference.

This issue:

#define __thread __declspec(thread)

was already fixed by this definition @ common.h:317

#if defined(_MSC_VER)
#define GOOGLE_THREAD_LOCAL __declspec(thread)
#else
#define GOOGLE_THREAD_LOCAL __thread
#endif

in commit by you
on 12/05/2014 SHA-1: d778778 "Fix thread local annotatoin and add back type traits is_convertable for MSVC", although it still does not valid for iOS build, because iOS does not support __thread attribute.

Another issue:

    #ifdef _MSC_VER
        using std::tr1::integral_constant;
        using std::tr1::is_convertible;
        using std::tr1::is_enum;
    #else
    ...

was fixed by you, too
on 12/05/2014 SHA-1: be20ae0 "Fix compile issues and test failures in VS2008."

The last issue (VS2008 error example below):

    error C2512: 'wl::protobuf::`anonymous-namespace'::valkeyOneofInstance' : no appropriate default constructor available

was fixed, but I still wondering why this was happening.
The one element in struct $name$OneofInstance that was needed a constructor is the one with type ::google::protobuf::internal::ArenaStringPtr and it compiles well if we remove const before the member with this type. Here is the screenshots: compiler error place and struct declaration.

So, my pull request became really short. Please excuse me for the inconvenience and not updating to the latest alpha release before commenting (too much work with lots of third-party libraries requires to think twice before updating to the latest release, that's why I had fixed issues with commit I checked out in November instead of keep tracking the development. Sorry.)

P.S. I'm using VS2008 (cl.exe version 15.00.30729.01) and VS2013.

@slavanap slavanap closed this Jan 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants