Page MenuHomePhabricator

[objcopy] Error when --preserve-dates is specified with standard streams
ClosedPublic

Authored by abrachet on Jun 10 2019, 12:45 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

abrachet created this revision.Jun 10 2019, 12:45 PM
abrachet marked 2 inline comments as done.Jun 10 2019, 12:50 PM
abrachet added inline comments.
llvm/test/tools/llvm-objcopy/ELF/invalid-preserve-dates.test
8 ↗(On Diff #203875)

These lines were getting very long. I tried to split it up with '\' but it didn't work. Is there another way to do this or is it fine to have very long lines?

llvm/tools/llvm-objcopy/CopyConfig.cpp
743 ↗(On Diff #203875)

I switched this to StringRef so std::find isn't comparing pointers

rupprecht accepted this revision.Jun 10 2019, 12:56 PM
rupprecht added inline comments.
llvm/test/tools/llvm-objcopy/ELF/invalid-preserve-dates.test
8 ↗(On Diff #203875)

You should be able to use \. Maybe you forgot to start the continuation line with RUN:? That's what was happening to me when I couldn't get it to work. See bad-output-format.test as an example.

There's no rule for short lines in tests, but when they get too long (>100 columns), I find it more readable to split them up around 80 columns.

This revision is now accepted and ready to land.Jun 10 2019, 12:56 PM
abrachet updated this revision to Diff 203884.Jun 10 2019, 1:05 PM

Split up long lines in invalid-preserve-dates.test

abrachet marked 2 inline comments as done.Jun 10 2019, 11:56 PM
abrachet added inline comments.
llvm/test/tools/llvm-objcopy/ELF/invalid-preserve-dates.test
8 ↗(On Diff #203875)

Yup, that's what I was missing. Thanks, Jordan!

jhenderson added inline comments.Jun 11 2019, 1:53 AM
llvm/test/tools/llvm-objcopy/ELF/invalid-preserve-dates.test
1 ↗(On Diff #203884)

A brief explanation at the start of the file as to what this test file is covering would be useful.

4 ↗(On Diff #203884)

Add a full stop to the end of comments, and use '##' to indicate they are comments for new tests.

7 ↗(On Diff #203884)

I find it easier to read if the follow-on lines are indented for a long RUN command like this:

# RUN: not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o - < \
# RUN:   %p/Inputs/alloc-symtab.o 2>&1 | FileCheck %s

Depending on the length of the main command, it is often easier to reflow the lines at the end of a command too:

# RUN: not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o - < %p/Inputs/alloc-symtab.o 2>&1 \
# RUN:   | FileCheck %s
abrachet updated this revision to Diff 204681.Jun 13 2019, 6:52 PM
abrachet marked an inline comment as done.

Added a comment to test case explaining its purpose. Also reformatted long RUN: lines.

abrachet marked 3 inline comments as done.Jun 13 2019, 6:52 PM
This revision was automatically updated to reflect the committed changes.