r212396 - PlistSupport.h: avoid gcc 'defined but not used' warning

[David Blaikie]

On Thu, Jul 10, 2014 at 7:14 AM, Alp Toker <alp at nuanti.com> wrote:
>
> On 08/07/2014 00:05, David Blaikie wrote:
>>
>> On Sun, Jul 6, 2014 at 12:59 AM, Alp Toker <alp at nuanti.com> wrote:
>>>
>>> Author: alp
>>> Date: Sun Jul  6 02:59:14 2014
>>> New Revision: 212396
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=212396&view=rev
>>> Log:
>>> PlistSupport.h: avoid gcc 'defined but not used' warning
>>
>> Is this warning helpful? Should we just disable it? I assume Clang's
>> warning here is smart enough not to warn for things in headers?
>
>
> I like this GCC warning and think clang's being a bit too clever here
> actually.

The general attitude/approach we try to take is to either fix clang or
disable the GCC warning. I suspect this would be hard to justify a fix
in clang (due to false positives - in the sense of "these are not
bugs, as such...").

But I agree that statics in headers aren't ideal - I suspect in the
clang world this would end up as a clang-tidy thing. (it's sort of
awkward not being able to raise the bar with clang warnings - one day
we'll get something like clang-tidy integrated in as optional
plugin/something with clang so we can choose a higher bar than only
those things that meet clang's on-by-default policy)

>>> Modified:
>>>      cfe/trunk/include/clang/Basic/PlistSupport.h
>>>      cfe/trunk/lib/ARCMigrate/PlistReporter.cpp
>>>      cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
>>>
>>> Modified: cfe/trunk/include/clang/Basic/PlistSupport.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/PlistSupport.h?rev=212396&r1=212395&r2=212396&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/PlistSupport.h (original)
>>> +++ cfe/trunk/include/clang/Basic/PlistSupport.h Sun Jul  6 02:59:14 2014
>>> @@ -19,12 +19,6 @@ namespace clang {
>>>   namespace markup {
>>>   typedef llvm::DenseMap<FileID, unsigned> FIDMap;
>>>
>>> -static const char *PlistHeader =
>>> -    "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
>>> -    "<!DOCTYPE plist PUBLIC \"-//Apple Computer//DTD PLIST 1.0//EN\" "
>>> -    "\"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"
>>> -    "<plist version=\"1.0\">\n";
>>> -
>>>   static inline void AddFID(FIDMap &FIDs, SmallVectorImpl<FileID> &V,
>>>                             const SourceManager &SM, SourceLocation L) {
>>>     FileID FID = SM.getFileID(SM.getExpansionLoc(L));
>>> @@ -49,6 +43,15 @@ static inline raw_ostream &Indent(raw_os
>>>     return o;
>>>   }
>>>
>>> +static inline raw_ostream &EmitPlistHeader(raw_ostream &o) {
>>
>> I don't think this function needs to be static, does it? If it's just
>> "inline" then it can be deduped by the linker, etc. (if it happens to
>> not get inlined)
>
>
> Addressed r212494.

Thanks!

>> Also, it might be simpler just to have the function return the string
>> for use, rather than making it so the string can only be used for
>> ostream emission like this.
>
> The encapsulation is intentional. Without conscious effort this is moving
> towards and efficient key-value emission interface that might some day build
> C API dictionaries etc. instead of plist strings.

Ah, fair enough - my suggestions were marginal/no-perfect-answer
anyway, and I did notice that using a function made it consistent with
other operations (like emitting integers, etc).

> In the meantime, wrapping ostream in this way encourages escaping which the
> old code often got wrong while attempting to pipe raw strings into the
> output file.

Hmm - not sure which old code you're referring to (how far back I'd
have to go in commit history, etc - I have a memory of a fish most of
the time, so I don't recall the higher level/other nearby changes
that've been made here recently).

>> Or should we just do the obvious fix and
>> declare the string in the header and define it in the implementation
>> file, as usual?
>
>
> I'd like to keep this facility purely header-based. It's just a collection
> of standalone functions that used to be copy-and-pasted, and not a pattern
> I'm prepared to formalize just yet.
>
> On the other hand, it's currently mislayered for modules and needs moving
> away out of include/Basic. I'm tracking this.

Hmm - it's actually broken under modules (what's the conflict?), or
just poor/sub-optimal layering?

- David

>
> Alp.
>
>
>
>>
>>> +  static const char *PlistHeader =
>>> +      "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
>>> +      "<!DOCTYPE plist PUBLIC \"-//Apple Computer//DTD PLIST 1.0//EN\" "
>>> +      "\"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"
>>> +      "<plist version=\"1.0\">\n";
>>> +  return o << PlistHeader;
>>> +}
>>> +
>>>   static inline raw_ostream &EmitInteger(raw_ostream &o, int64_t value) {
>>>     o << "<integer>";
>>>     o << value;
>>>
>>> Modified: cfe/trunk/lib/ARCMigrate/PlistReporter.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/PlistReporter.cpp?rev=212396&r1=212395&r2=212396&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/ARCMigrate/PlistReporter.cpp (original)
>>> +++ cfe/trunk/lib/ARCMigrate/PlistReporter.cpp Sun Jul  6 02:59:14 2014
>>> @@ -63,8 +63,7 @@ void arcmt::writeARCDiagsToPlist(const s
>>>       return;
>>>     }
>>>
>>> -  // Write the plist header.
>>> -  o << PlistHeader;
>>> +  EmitPlistHeader(o);
>>>
>>>     // Write the root object: a <dict> containing...
>>>     //  - "files", an <array> mapping from FIDs to file names
>>>
>>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=212396&r1=212395&r2=212396&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)
>>> +++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Sun Jul  6
>>> 02:59:14 2014
>>> @@ -345,8 +345,7 @@ void PlistDiagnostics::FlushDiagnosticsI
>>>       return;
>>>     }
>>>
>>> -  // Write the plist header.
>>> -  o << PlistHeader;
>>> +  EmitPlistHeader(o);
>>>
>>>     // Write the root object: a <dict> containing...
>>>     //  - "clang_version", the string representation of clang version
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> --
> http://www.nuanti.com
> the browser experts
>