This is an archive of the discontinued LLVM Phabricator instance.

Fixed a ton of gcc compile warnings
ClosedPublic

Authored by vharron on May 11 2015, 12:03 PM.

Details

Summary

Removed some unused variables, added some consts, changed some casts
to const_cast. I don't think any of these changes are very
controversial.

Diff Detail

Repository
rL LLVM

Event Timeline

vharron updated this revision to Diff 25493.May 11 2015, 12:03 PM
vharron retitled this revision from to Fixed a ton of gcc compile warnings.
vharron updated this object.
vharron edited the test plan for this revision. (Show Details)
vharron added reviewers: chaoren, labath, clayborg.
vharron added a subscriber: Unknown Object (MLST).
chaoren edited edge metadata.May 11 2015, 12:29 PM

I think it's a good idea to use static_cast/static_pointer_cast/reinterpret_cast over C-style casts.

labath accepted this revision.May 12 2015, 1:37 AM
labath edited edge metadata.

It probably does not make sense to go in and change everything again, but I would also prefer C++ casts next time - it makes things more explicit and easier to review. LGTM.

This revision is now accepted and ready to land.May 12 2015, 1:37 AM
clayborg edited edge metadata.May 12 2015, 10:28 AM

Looks good overall. I don't like removing "const". Why does GCC warn on the first arg "const unsigned char* ptr"?

static inline void
WriteSwappedInt64(const unsigned char* ptr, unsigned offset, uint64_t value)
include/lldb/Core/DataEncoder.h
359–360 ↗(On Diff #25494)

Why do we need to remove const here?

source/Core/DataEncoder.cpp
24–53 ↗(On Diff #25494)

Why do we need to remove const here?

156 ↗(On Diff #25494)

Why do we need to remove const here?

vharron added inline comments.May 12 2015, 2:47 PM
include/lldb/Core/DataEncoder.h
359–360 ↗(On Diff #25494)

That buffer is the one that data encoder writes to. Internally, it is stored as a non-const pointer to enable these writes. I think it's wrong to provide a const guarantee in the interface when we know we're going to modify it.

source/Core/DataEncoder.cpp
24–53 ↗(On Diff #25494)

ptr is going to be modified so we shouldn't be providing a const guarantee.

156 ↗(On Diff #25494)

See first comment

"ptr" isn't being modified in any of the WriteInt* functions...

source/Core/DataEncoder.cpp
53 ↗(On Diff #25494)

How is "ptr" modified here? It is just casted and used in pointer math, but it isn't being modified...

clayborg accepted this revision.May 12 2015, 2:59 PM
clayborg edited edge metadata.

Never mind.. It is writing to the memory... I was thinking read, not with. Ignore me.

This revision was automatically updated to reflect the committed changes.