Details
Diff Detail
- Build Status
Buildable 7310 Build 7310: arc lint + arc unit
Event Timeline
include/clang/Frontend/PrecompiledPreamble.h | ||
---|---|---|
248 | Is there a missing declaration here or is this a comment for another declaration? |
include/clang/Frontend/PrecompiledPreamble.h | ||
---|---|---|
248 | Indeed, I forgot to remove a comment after I removed a declaration. |
include/clang/Frontend/PrecompiledPreamble.h | ||
---|---|---|
43–44 | a) s/PCHTempFile/TempPCHFile/ | |
98 | I'd initialize these. | |
125 | If a user doesn't care about the things above this class, can we move those into an extra header? | |
143 | the lifetime ... stores a PCH | |
158–161 | Why not make it a member instead? | |
lib/Frontend/ASTUnit.cpp | ||
98–101 | I'd instead just have both a unique_ptr for memory management and a pointer (for unowned access). Then, if owned, make the raw pointer point to the unique_ptr, if unowned, the unique_ptr stays nullptr. | |
131–136 | If this indeed needs to return a possibly owned buffer, explain when exactly it is owned and when it is unowned. |
- Added default initializers to PreambleFileHash.
- Update file comments to address klimek's questions.
include/clang/Frontend/PrecompiledPreamble.h | ||
---|---|---|
43–44 | a) Done. b) I would even call this class 'DeleteFileGuard' and leave only createFromCustomPath method to construct it. I assume it would only remove files when someone 'forgets' to call a destructor (i.e. leaks a heap object) of PrecompiledPreamble (ASTUnit in the older version of the code). If that's the case, I would go for removing the static map altogether and clearing up the memory leaks instead. But if there are other cases where it would delete files I'm not aware of, we might want to keep this code. In any case, I don't want this discussion to block this patch, I would rather start it after the patch lands. | |
125 | Do you have any suggestions of where to put it and how to call it? | |
158–161 | To keep BuildPreamble, CanReusePreamble and AddImplicitPreamble close to each other. We could make all of those members, of course. Do you think that would make API better? | |
lib/Frontend/ASTUnit.cpp | ||
98–101 | It looks like a mess when you return it from a function. (i.e. should we return a pair<unique_ptr<T>, T*> or accept an out parameter unique_ptr<T> &Owner and return T*?) The function itself wasn't there before, it was extracted as a part of this refactoring. This is essentially the same thing with a few helper methods to make code using it more readable. | |
131–136 | Done. It's owned when read from disk and not owned when taken from CompilerInvocation.PPOptions' file-to-buffer remappings. |
lib/Frontend/ASTUnit.cpp | ||
---|---|---|
131–136 | After some in person discussion, the idea now is to just always copy and return a unique_ptr |
Removed PossiblyOwnedBuffer, added an extra copy instead.
This makes the code much simpler.
include/clang/Frontend/PrecompiledPreamble.h | ||
---|---|---|
125 | TempPCHFile looks like something we just want to put into the .cc file and store as a unique_ptr. | |
158–161 | Generally, if these are closely coupled to implementation details of PrecompiledPreample, I think that coupling is strong enough to warrant making them members. |
- Made TempPCHFile an inner class of PrecompiledPreamble.
- Made PreambleFileHash an inner class of PrecompiledPreamble.
- Changed stanalone functions to members.
- Removed some member accessors that were no longer used.
include/clang/Frontend/PrecompiledPreamble.h | ||
---|---|---|
125 | Made both inner classes of PrecompiledPreamble instead. | |
158–161 | Made all three useful functions members(BuildPreamble is now a static member, too, called PrecompiledPreamble::Build). Also removed some accessors that weren't used outside those functions. |
a) s/PCHTempFile/TempPCHFile/
b) is the unique-ness the only thing that's special? Perhaps UniqueTempFile or somesuch? Seems nothing PCH-specific is here to see
c) why don't we want to support VFS's here?