Commit de0c0b83 authored by Albert Astals Cid's avatar Albert Astals Cid
Browse files

FileSpec: Move the fileSpec.dictLookup call inside fileSpec.isDict if

Fixes #704
parent 653d7437
Pipeline #13468 passed with stage
in 7 minutes and 35 seconds
......@@ -133,11 +133,12 @@ FileSpec::FileSpec(const Object *fileSpecA)
obj1 = fileSpec.dictLookup("Desc");
if (obj1.isString())
desc = obj1.getString()->copy();
obj1 = fileSpec.dictLookup("Desc");
if (obj1.isString()) {
  • @aacid Reading and writing several checks like this, I wonder if we should add methods like

    const GooString *Object::asString() const {
      return type == objString ? string : nullptr;

    to Object so that these could be simplified to something like

    if (const GooString *str = obj1.asString()) {
      desc = str->copy();

    This pattern should also make the array* and dict* methods of Object unnecessary in the long run. What do you think?

  • To be honest it doesn't seem a big improvement to me, we had an if before, we still have an if now

  • we still have an if now

    We do, but it is a C++ poor man's pattern matching if. I's say that main point is to destructure a complex type like Object, i.e. inside of the if a new binding (str in the above case) is in scope which will be used instead of the original Object. This clarifies intent and makes it harder to forget the checking compared to the plain getter.

  • Sure, it's marginally better, and i wouldn't be against introducing it bit by bit where it makes sense, but not sure doing a big rewrite all at once makes sense

  • but not sure doing a big rewrite all at once makes sense

    On this point I very much agree. Will try to prepare something when time permits...

Please register or sign in to reply
desc = obj1.getString()->copy();
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment