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

Fix warnings produced when compiling with clang #14

Merged
merged 8 commits into from
Aug 22, 2016
Merged

Fix warnings produced when compiling with clang #14

merged 8 commits into from
Aug 22, 2016

Conversation

pefoley2
Copy link
Contributor

@pefoley2 pefoley2 commented May 5, 2016

No description provided.

@wrwilliams
Copy link
Member

Is the DynC regeneration also for clang warning cleanup?

@pefoley2
Copy link
Contributor Author

pefoley2 commented May 5, 2016

Yes, it was throwing a lot of warnings about the deprecated register keyword, and regenerating the parser/lexer with the latest bison/flex versions seemed like the best solution.

@wrwilliams
Copy link
Member

Great. Let's get the clang and CI moving then, and this can come in on top of those.

@pefoley2
Copy link
Contributor Author

pefoley2 commented May 5, 2016

Rebased onto the clang pull request.

@pefoley2
Copy link
Contributor Author

pefoley2 commented May 6, 2016

Depends on #13

@pefoley2
Copy link
Contributor Author

pefoley2 commented May 9, 2016

Now that #13 has been merged, this PR is ready for review and merging.

@pefoley2
Copy link
Contributor Author

pefoley2 commented May 9, 2016

On second thought, this can wait until after the 9.2 release.

@pefoley2
Copy link
Contributor Author

Now that 9.2 has been released, this is ready for review.

@wrwilliams
Copy link
Member

What's up with the warnings about comparing typeids? That's in there for speed; if there's an accuracy problem obviously that needs fixing...

@pefoley2
Copy link
Contributor Author

pefoley2 commented Jul 5, 2016

Clang produces this warning for usages of typeid(*var): warning: expression with side effects will be evaluated despite being used as an operand to 'typeid' [-Wpotentially-evaluated-expression]

I'm not conversant enough with RTTI to be able to safely determine if this warning can be ignored.

@pefoley2
Copy link
Contributor Author

pefoley2 commented Jul 5, 2016

It looks like https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaExprCXX.cpp#L451 might be where the warning is coming from, but I'm not totally sure.

@pefoley2
Copy link
Contributor Author

pefoley2 commented Jul 5, 2016

It appears that llvm-mirror/clang@f1a2b53 is the commit that introduced the warning.

@wrwilliams
Copy link
Member

I think those warnings are safe in our context--the operation in question is a shared_ptr dereference the majority of the time. From what I've seen in profiling typeid is faster in the cases where we have what we expect (and not by a small amount). That may have changed, though--the last time I profiled that code was years ago and with IIRC gcc 4.1.2. If there's no longer a practical speed advantage to typeid, we may as well use dynamic_cast and make the compiler happy.

@pefoley2
Copy link
Contributor Author

pefoley2 commented Jul 7, 2016

So is this ok for merging or do I need to rework anything?

@wrwilliams
Copy link
Member

Should be okay now; I'll let you know if it's not. Letting this one be a test case for my Jenkins setup, if you don't mind, though.

@pefoley2
Copy link
Contributor Author

pefoley2 commented Jul 7, 2016

Sure, no rush.

@wrwilliams
Copy link
Member

Clean under Jenkins, though it's not updating status correctly.

@wrwilliams wrwilliams merged commit 2d0b0b4 into dyninst:master Aug 22, 2016
@cuviper
Copy link
Contributor

cuviper commented Aug 25, 2016

RHEL6's GCC 4.4 doesn't support these new nullptrs in instructionAPI/src/BinaryFunction.C. Isn't that still a supported target?

@wrwilliams
Copy link
Member

It is, and I'm not sure why my Jenkins setup (which should be using system gcc) didn't catch it.

@jdetter
Copy link
Contributor

jdetter commented Aug 25, 2016

The system gcc (4.4.7) on Follis is complaining about it for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants