Repository: uber-go/zap
File: sugar.go (Go)

Code before:
// Fatalw logs a message with some additional context, then calls os.Exit. The
// variadic key-value pairs are treated as they are in With.
func (s *SugaredLogger) Fatalw(msg string, keysAndValues ...interface{}) {
	s.log(FatalLevel, msg, keysAndValues)
}

func (s *SugaredLogger) log(lvl zapcore.Level, msg string, context []interface{}) {
	if ce := s.core.Check(lvl, msg); ce != nil {
		ce.Write(s.sweetenFields(context)...)
	}
}

func (s *SugaredLogger) sweetenFields(args []interface{}) []zapcore.Field {
	if len(args) == 0 {
		return nil
	}

	// Allocate enough space for the common case; if users only pass structured
	// fields, we'll need to grow the slice.
	fields := make([]zapcore.Field, 0, len(args)/2)
	var i int
	for i < len(args) {
		if f, ok := args[i].(zapcore.Field); ok {
			fields = append(fields, f)
			i++
			continue
		}
		if i == len(args)-1 {
			s.core.DPanic(_oddNumberErrMsg, Any("ignored", args[i]))
			break
		}
		key, ok := args[i].(string)
		val := args[i+1]
		if !ok {
			s.core.DPanic(
				_nonStringKeyErrMsg,
				Int("position", i),
				Any("key", args[i]),
				Any("value", val),
			)
			key = fmt.Sprint(args[i])
		}
		fields = append(fields, Any(key, val))
		i += 2
	}

	return fields
}


Review comment (bug):
Can we make the state machine structure clearer please? Something like:
```
for i < len(args) {
  // check for inlined zap Fields
  if f, ok := ... { ... }

  // parity check: we need an even number of key/val pairs (at least since the last zap Field)
  if i == len(args)-1 { ... }]

  // consume a key/val pair
  key, val := args[i], args[i+1]

  // process valid string key
  if keyString, ok := key.(string); ok {
    fields = append(fields, Any(keyString, val))
    i += 2
   continue
  }

  // we have a "key" that isn't a string, crash in development...
  s.core.DPanic(...)

  // ...but in production, stop field conversion here, since this could be a parity error (e.g. a forgotten key, and not the optimistic "you forgot to string-convert a key")
  break
}
```

You could, of course, flip the `string` cast ok predicate back to the negative.

Also see if you think:
```
key := args[i]
i++

val := args[i]
i++
```

is any better; the biggest downside I can see is a need to `i-2` or `i-1` inside the DPanic site, depending on how you intersperse the key/val extraction with the key-string check.

In short: I bet a lot of people will end up reading this source code, since it's one step beneath a common entry point for any `godef` chasers; let's optimize its readability, not just to convince ourselves that it's correct / robust.

Code after:

// Fatalw logs a message with some additional context, then calls os.Exit. The
// variadic key-value pairs are treated as they are in With.
func (s *SugaredLogger) Fatalw(msg string, keysAndValues ...interface{}) {
	s.log(FatalLevel, msg, keysAndValues)
}

func (s *SugaredLogger) log(lvl zapcore.Level, msg string, context []interface{}) {
	if ce := s.core.Check(lvl, msg); ce != nil {
		ce.Write(s.sweetenFields(context)...)
	}
}

func (s *SugaredLogger) sweetenFields(args []interface{}) []zapcore.Field {
	if len(args) == 0 {
		return nil
	}

	// Allocate enough space for the worst case; if users pass only structured
	// fields, we shouldn't penalize them with extra allocations.
	fields := make([]zapcore.Field, 0, len(args))
	var invalid invalidPairs

	for i := 0; i < len(args); {
		// This is a strongly-typed field. Consume it and move on.
		if f, ok := args[i].(zapcore.Field); ok {
			fields = append(fields, f)
			i++
			continue
		}

		// Make sure this element isn't a dangling key.
		if i == len(args)-1 {
			s.core.DPanic(_oddNumberErrMsg, Any("ignored", args[i]))
			break
		}

		// Consume this value and the next, treating them as a key-value pair. If the
		// key isn't a string, add it to the slice of invalid pairs.
		key, val := args[i], args[i+1]
		if keyStr, ok := key.(string); !ok {
			// Subsequent errors are likely, so allocate once up front.
			if cap(invalid) == 0 {
				invalid = make(invalidPairs, 0, len(args)/2)
			}
			invalid = append(invalid, invalidPair{i, key, val})
		} else {
			fields = append(fields, Any(keyStr, val))
		}
		i += 2
	}