Adding conditional compiler symbol to support .NET 3.5 #1713
Conversation
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! On Fri, Jun 24, 2016 at 9:26 AM, googlebot notifications@github.com wrote:
Detlev Schwabe Cell: +1 310 699 8693 |
|
CLAs look good, thanks! |
| #if DOTNET35 | ||
| private static Dictionary<object, string> GetNameMapping(System.Type enumType) => | ||
| enumType.GetFields(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Static) | ||
| .Where(f => f.IsStatic) |
There was a problem hiding this comment.
You can get rid of BindingFlags.Instance above and this Where clause.
|
Thanks for this. A few nits, and we need ReadOnlyDictionary before it'll work. |
|
Since this is my very first time working with GitHub, how will I get my additional local changes up into my branch that I created on my fork, and then from there into the current PR? |
|
You should commit your changes locally on the same branch, push to the same branch in your fork, and then they'll become part of this pull request. When everything looks okay, we'll squash the multiple commits down to a single one to merge onto this repo. |
| @@ -49,6 +49,11 @@ internal static class TypeExtensions | |||
| /// Returns true if the target type is a value type, including a nullable value type or an enum, or false | |||
| /// if it's a reference type (class, delegate, interface - including System.ValueType and System.Enum). | |||
| /// </summary> | |||
| #if DOTNET35 | |||
| internal static bool IsValueType(this Type target) { | |||
There was a problem hiding this comment.
Gah, I missed the brace-at-end-of-line here. Not to worry - will fix that up later myself.
|
Looks like the Travis failure is unrelated to this. Squashing and merging now. |
|
@detlevschwabe I was only able to get this to work with .NET 3.5 after changing the DOTNET35 symbols to NET35. Are you sure DOTNET35 is correct? |
|
Valid preprocessor symbols are listed at https://docs.microsoft.com/en-us/dotnet/articles/core/tutorials/libraries
|
|
@JohnHBrock: Those are just the automatically-applied symbols. It's valid to make a build define others. |
|
Since NET35 is automatically defined, I ended up creating a PR (#2715) to change the DOTNET35 symbols to NET35 in the interest of making things work out-of-the-box (the PR also includes some other fixes for enabling .NET 3.5 support). |
|
My solution was to keep a separate .csproj file with DOTNET35 defined.
If you think NET35 is set exclusively for builds under .NET 3.5 then go
ahead with your PR. Please ask Jon Skeet for a review.
Not sure if you're aware of this, but the .NET35 build has been broken for
a while now.
This
fb71df9
is causing this compile error:
ByteString.cs(160,20): error CS1061: 'Stream' does not contain a definition
for 'CopyTo' and no extension method 'CopyTo' accepting a first argument of
type 'Stream' could be found
That is because Stream in .NET35 does not have a CopyTo method to another
stream.
This could be fixed by rewriting this part for NET35 and for instance use a
snippet like this:
public static void CopyStream(Stream input, Stream output){
byte[] buffer = new byte[32768];
int read;
while ((read = input.Read(buffer, 0, buffer.Length)) > 0)
{
output.Write (buffer, 0, read);
}}
If you feel like fixing it, go ahead.
On Tue, Feb 21, 2017 at 2:38 PM John Brock ***@***.***> wrote:
Since NET35 is automatically defined, I ended up creating a PR (#2715
<#2715>) to change the DOTNET35
symbols to NET35 in the interest of making things work out-of-the-box (the
PR also includes some other fixes for enabling .NET 3.5 support).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1713 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ATLiQtzbWYzIOr6sarAzVNETZ7Or1Z9hks5re2eAgaJpZM4I965N>
.
--
Detlev Schwabe
13230 Fiji Way
Unit B
Marina Del Rey, CA 90292
|
|
I actually already fixed that Stream.CopyTo(...) issue as part of my pull request. |
No description provided.