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.

Sign up for GitHub

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

Jump to bottom

Enable extra warnings #109

Merged
merged 1 commit into from Sep 30, 2017
Merged

Enable extra warnings #109

merged 1 commit into from Sep 30, 2017

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented Sep 28, 2017

These I believe are set to the default with CMake (maybe not pedantic).

@dnfclas
Copy link

dnfclas commented Sep 28, 2017

@hughbe,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@hughbe hughbe mentioned this pull request Sep 28, 2017
Open
akoeplinger
akoeplinger requested changes Sep 28, 2017
View reviewed changes
src/Makefile.am Outdated
@@ -121,6 +121,6 @@ libgdiplus_la_SOURCES = \

libgdiplus_la_LIBADD = $(GDIPLUS_LIBS)

AM_CPPFLAGS = $(GDIPLUS_CFLAGS) -Wall -Wno-unused -Wno-format
AM_CPPFLAGS = $(GDIPLUS_CFLAGS) -Wall -Wno-unused -Wno-format -pedantic
Copy link
Member

Choose a reason for hiding this comment

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

this isn't necessary since it's already included in GDIPLUS_CFLAGS

configure.ac Outdated
@@ -29,7 +29,7 @@ PKG_CHECK_MODULES(CAIRO, cairo >= $CAIRO_REQUIRED_VERSION)
# Optional use (experimental and unsupported) of Pango's text rendering on top of Cairo
AC_ARG_WITH(pango, [ --with-pango],[text_v=pango],[text_v=cairo])

GDIPLUS_CFLAGS="$GDIPLUS_CFLAGS -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare"
GDIPLUS_CFLAGS="$GDIPLUS_CFLAGS -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -pedantic"
Copy link
Member

Choose a reason for hiding this comment

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

according to the Linux build failure we need to add -std=c99 so it doesn't default to c89 and complains about ancient things.

@hughbe hughbe force-pushed the extra-warnings branch 2 times, most recently from f4fae86 to 1665e20 Compare September 28, 2017 20:57
@akoeplinger
Copy link
Member

akoeplinger commented Sep 28, 2017 •
edited

@hughbe ok so I looked at the Linux build and there are two classes of warnings left that get triggered by pedantic:

bmpcodec.c:1186:4: error: ISO C forbids conversion of object pointer to function pointer type [-Werror=pedantic]
    ((PutBytesDelegate)(pointer))(data, size); 
     ^

I can't wrap my head around these at the moment, any ideas?

The other one are warnings like:

testpen.c: In function 'test_setPenMode':
testpen.c:1301:2: error: overflow in implicit constant conversion [-Werror=overflow]
  assertEqualInt (mode, (PenAlignment)(PenAlignmentCenter - 1));
  ^

That one makes more sense to me and is fixed by removing the cast to PenAlignment.

Do you think we should look into this further or is it "too pedantic" at this point? 😄

If I fix the overflow warnings and uncomment the locations where the *BytesDelegate are used then it compiles to the end at least.

@hughbe
Copy link
Contributor Author

hughbe commented Sep 30, 2017

I agree. Pedantic is certainly unecessary for the unit tests. For the product code it may be good. But it looks unecessary. I reverted this change but fixed some of the warnings pedantic wuld have found.

@akoeplinger akoeplinger merged commit 22e771c into mono:master Sep 30, 2017
@hughbe hughbe deleted the extra-warnings branch September 30, 2017 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akoeplinger akoeplinger akoeplinger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hughbe @dnfclas @akoeplinger